Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop json rpc gateway support #583

Merged
merged 7 commits into from
Feb 10, 2022
Merged

Drop json rpc gateway support #583

merged 7 commits into from
Feb 10, 2022

Conversation

mmlb
Copy link
Contributor

@mmlb mmlb commented Jan 25, 2022

Description

Drop all code dealing with json rpc.

Why is this needed

Its likely unused code which has some weird code (:eye: runtime.String) that make other PRs more tedious (#567, also I want to drop the self-signed /cert stuff too) to implement.

How Has This Been Tested?

go build and CI.

How are existing users impacted? What migration steps/scripts do we need?

No impact I suspect (and hope). I know of 3 other tink clients and they all use grpc:

@mmlb
Copy link
Contributor Author

mmlb commented Jan 25, 2022

I realized you have a dummy client @micahhausler and we didn't clarify it in slack so pinging you here. I hope its grpc based :D.

@micahhausler
Copy link
Contributor

I realized you have a dummy client @micahhausler and we didn't clarify it in slack so pinging you here. I hope its grpc based :D.

The virtual worker just uses the gRPC interface like tink-worker. No risk here from me

@codecov
Copy link

codecov bot commented Jan 25, 2022

Codecov Report

Merging #583 (abdda5c) into main (f704d1c) will increase coverage by 4.91%.
The diff coverage is n/a.

❗ Current head abdda5c differs from pull request most recent head 740cfec. Consider uploading reports for the commit 740cfec to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #583      +/-   ##
==========================================
+ Coverage   38.48%   43.39%   +4.91%     
==========================================
  Files          53       51       -2     
  Lines        3565     3104     -461     
==========================================
- Hits         1372     1347      -25     
+ Misses       2096     1671     -425     
+ Partials       97       86      -11     
Impacted Files Coverage Δ
http-server/http_server.go

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f704d1c...740cfec. Read the comment docs.

micahhausler
micahhausler previously approved these changes Jan 25, 2022
Copy link
Contributor

@micahhausler micahhausler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay for cutting code!

cmd/tink-server/main.go Show resolved Hide resolved
.envrc Show resolved Hide resolved
Copy link

@cprivitere cprivitere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good other than wondering if the .envrc will work as written as I don't know that dotenv_if_exists is a standard function (doesn't seem to be on my machines).

@mmlb
Copy link
Contributor Author

mmlb commented Feb 8, 2022

Yep it should, maybe you need to update? https://direnv.net/man/direnv-stdlib.1.html#codedotenvifexists-ltdotenvpathgtcode

cprivitere
cprivitere previously approved these changes Feb 8, 2022
Make use of direnv's stdlib having `has` which replaces the old `which ... &>/dev/null`.
This also sources env vars from .env files which should not be added to git and allow for
local user to override/add things without git trying to track it.
Finally, add ./bin to both PATH and GOBIN to avoid contamination of global GOBIN.

Signed-off-by: Manuel Mendez <[email protected]>
lg is already of copy of the arg so theres no need to create a function variable
to copy the arg when just naming it logger would do.

Signed-off-by: Manuel Mendez <[email protected]>
Yay one global variable down, many more to go.

Signed-off-by: Manuel Mendez <[email protected]>
Signed-off-by: Manuel Mendez <[email protected]>
Pretty sure no one is using this over grpc rpc support and thus is a lot of
useless complexity. So lets get rid of unused code!

Signed-off-by: Manuel Mendez <[email protected]>
http_handlers.go only contained the logic for translating json rpc -> grpc in
the Register{Hardware,Template,Workflow}ServiceHandlerFromEndpoint functions.
Since we don't need them they can go away. Since there's not http_handlers.go
we don't need http_handlers_test.go either.

Signed-off-by: Manuel Mendez <[email protected]>
These are no longer needed.

Signed-off-by: Manuel Mendez <[email protected]>
Copy link
Contributor

@markjacksonfishing markjacksonfishing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
I love when we shed code so thank you for this!

@mmlb mmlb added the ready-to-merge Signal to Mergify to merge the PR. label Feb 10, 2022
@mergify mergify bot merged commit fc76c03 into tinkerbell:main Feb 10, 2022
@mmlb mmlb deleted the drop-json-rpc branch February 11, 2022 15:30
@displague displague added this to the 0.7.0 milestone Aug 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Signal to Mergify to merge the PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants