Bootstrap Get command for tink-cli #406

gianarb · 2020-12-28T15:31:03Z

Description

Create a reusable Get command.

TODO:

Implement field-selector to filter by some fields --filder-selector mac==aa:bb:cc:dd, --field-selector template==6b1ad3d5-3dd7-43be-9c08-8cc566cb0ae5. The goal here is to deprecate the hardware IP and Mac command. (we can't do it at this stage because the grpc API is not flexible enough, it will be a nightmare to implement)
Create a structure in the client pkg that contains all the grcp clients and pass it to get.CmdOpt instead of using the global clients
Write tests for the get command in hardware, template, and workflow
Have a look at how to use Get for events
Implement populate with args

Why is this needed

Related to #405

A good way to gain consistency across a CLI is to write code that enforces that. Right now we rewrite commands across resources and this practice is wrong.

This PR bootstraps a Get command that can be used across all the resources.

How Has This Been Tested?

Unit tests

How to migrate?

This PR breaks two commands:

$ tink template get
$ tink workflow get

You can get the same behavior setting the environment variable TINK_CLI_VERSION=0.0.0. Please, we will remove this workaround very soon, let us know what you have missed with the new command and we will try to give you an alternative that works with the new CLI we have in our mind!

There are a couple of deprecated commands that print to stderr the alternative you can use. Please let us know if some of your use cases are not covered yet

codecov · 2020-12-29T07:43:50Z

Codecov Report

Merging #406 (68a7185) into master (75bf4b6) will decrease coverage by 3.99%.
The diff coverage is 21.64%.

@@            Coverage Diff             @@
##           master     #406      +/-   ##
==========================================
- Coverage   38.34%   34.34%   -4.00%     
==========================================
  Files          31       46      +15     
  Lines        2355     2865     +510     
==========================================
+ Hits          903      984      +81     
- Misses       1376     1794     +418     
- Partials       76       87      +11

Impacted Files	Coverage Δ
cmd/tink-cli/cmd/hardware/delete.go	`0.00% <0.00%> (ø)`
cmd/tink-cli/cmd/hardware/id.go	`0.00% <0.00%> (ø)`
cmd/tink-cli/cmd/hardware/ip.go	`0.00% <0.00%> (ø)`
cmd/tink-cli/cmd/hardware/list.go	`0.00% <0.00%> (ø)`
cmd/tink-cli/cmd/hardware/mac.go	`0.00% <0.00%> (ø)`
cmd/tink-cli/cmd/hardware/push.go	`0.00% <0.00%> (ø)`
cmd/tink-cli/cmd/hardware/watch.go	`0.00% <0.00%> (ø)`
cmd/tink-cli/cmd/root.go	`27.27% <0.00%> (-10.23%)`	⬇️
cmd/tink-cli/cmd/template.go	`0.00% <0.00%> (-33.34%)`	⬇️
cmd/tink-cli/cmd/template/create.go	`0.00% <0.00%> (ø)`
... and 23 more

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 75bf4b6...68a7185. Read the comment docs.

thebsdbox · 2020-12-31T10:47:18Z

Nice I love this !

thebsdbox

/lgtm - I think the idea to sanitise the UX now is a great idea.

cmd/tink-cli/cmd/workflow/list.go

client/main.go

stolsma · 2020-12-31T12:33:26Z

@gianarb A generic Tink grpc client abstraction is also needed for the web Portal project (and maybe later on also for other projects) as that project doesn't work anymore because of changed Tink grpc code. Is it possible to carve the generic parts of Tink grpc client interactions out to a specific package? I'm referring here to the grpc code in client.go...

What do you think??

EDIT: Nevermind, thats what you are doing here... My brain is going to fast again, sorry.... :-|

gianarb · 2020-12-31T13:28:26Z

Thank you for your review @thebsdbox ! @stolsma thank you as well I will address your comment next week!

About the grpc client. The package is already isolated https://github.com/tinkerbell/tink/tree/master/client and it has its own interfaces but I don't like that it uses global variables. I would like to have a better-defined API (I think API suffers consistency issue just like the CLI and I hope we will be in the position to fix it (it means breaking changes)), but at least the code is there and isolated and reusable already.

This PR makes the first refactoring and offers an alternative without global variables. I will integrate, as part of this PR a few mocks that I hope will set a common way to use the client in a test suite.

is also needed for the web Portal project

Now I am curious about this! Do you know we have the first draft of a Tinkerbell portal https://github.com/tinkerbell/portal ? It does not work now as you mentioned because it uses an old API I think. @gauravgahlot made it a few months ago

stolsma · 2021-01-04T09:46:25Z

@gianarb Yep, I know the Tinkerbell Portal doesn't work at this moment because of the OLD API use and I already created a Issue for that (cubenix/tink-dashboard#16) before I found out that you where working on improving the Tink client API ;-)

Love your ideas about the client API direction, that's the direction I was hinting at in my previous comment before I scanned your code so you where already on it :-)

I would like to take a stab into implementing your improved client API into the Portal code to get that running again. When do you think the code of this pull request will be merged so I can use it there?

gianarb · 2021-01-04T09:51:27Z

Hey @stolsma ! Great, I am very happy to read that you want to help us and make a contribution. You have to give me a couple of days to figure out when @mmlb and a few other people will be back from vacation because as you imagine this is a good chunk of work coming from me directly, I want to discuss it with other contributors first.

Worst case I think we can open a PR with only the Client related work! That said I hope to have the client part merged this week.

client/main.go

gianarb · 2021-01-04T10:48:05Z

This CLI has currently one problem, the grpc connection gets created as part of the cmd/tink-cli.Main. it means that no matter what a tink-server has to be up and running even to get --help working. We have to fix it somehow, probably we should create the client later, I will have a look. I don't want to implement a Lazy client yet

displague · 2021-01-04T16:31:05Z

@gianarb have a look at equinix/metal-cli#74 which made a similar change to a decedent project.

That change is already in the docs and completions subcommand:

tink/cmd/tink-cli/cmd/completion.go

Line 53 in 1009dc3

return nil

client/main.go

cmd/tink-cli/cmd/get/get.go

cmd/tink-cli/cmd/hardware/get.go

cmd/tink-cli/cmd/hardware/ip.go

cmd/tink-cli/cmd/hardware/mac.go

cmd/tink-cli/cmd/workflow/list.go

mmlb · 2021-01-04T21:38:59Z

Well that was a doozy :D

mmlb · 2021-01-04T21:43:03Z

* [ ]  Implement `field-selector` to filter by some fields `--filder-selector mac==aa:bb:cc:dd`, --field-selector template==6b1ad3d5-3dd7-43be-9c08-8cc566cb0ae5`. The goal here is to deprecate the hardware IP and Mac command.

I don't think this would be very nice UX, why does Get need to support that at all? The Get implementation can just parse the rest of the args itself to figure out what to filter off of. And if you do the change to `NewGetCommand we can probably also add an extra arg to add filter examples too.

gianarb · 2021-01-05T13:23:25Z

I don't think this would be very nice UX, why does Get need to support that at all? The Get implementation can just parse the rest of the args itself to figure out what to filter off of. And if you do the change to `NewGetCommand we can probably also add an extra arg to add filter examples too.

@mmlb I think I don't understand what you are saying. Can you help me?

why does Get need to support that at all?

I would like to combine the various requests we have:

Get by ID
Get All
Get by IP
Get by MAC

All under the same get command. Same as kubectl does. The first two are easy:

Get By ID: tink <resource> get id1 id2 id3
Get All: tink <resource> get

For the other two? (or more if needed)

mmlb · 2021-01-05T14:39:59Z

I don't think this would be very nice UX, why does Get need to support that at all? The Get implementation can just parse the rest of the args itself to figure out what to filter off of. And if you do the change to `NewGetCommand we can probably also add an extra arg to add filter examples too.

@mmlb I think I don't understand what you are saying. Can you help me?

why does Get need to support that at all?

I would like to combine the various requests we have:
* Get by ID

* Get All

* Get by IP

* Get by MAC
All under the same get command. Same as kubectl does. The first two are easy:

Get By ID: tink <resource> get id1 id2 id3
Get All: tink <resource> get

For the other two? (or more if needed)

So I mean to make Get pluggable and/or just be the common "interface" of get commands. So in that case get uuid1 uuid1 or just get can be handled by Get but anything else that Get doesn't understand it passes to a function (like RetrieveData does, ParseGet or something?)

Does that clear it up?

gianarb · 2021-01-12T13:53:44Z

@displague about this:

#406 (comment)

I am not sure I like it unfortunately and but tried passing the client in a lot of different ways delaying as long as possible but I can't find a good way yet. I am confused atm, will get back on that problem later but all the other comments are addressed

mmlb · 2021-01-25T16:15:24Z

client/main.go

+
+func (o *ConnOptions) SetFlags(flagSet *pflag.FlagSet) {
+	flagSet.StringVar(&o.CertURL, "tinkerbell-cert-url", "http://127.0.0.1:42114/cert", "The URL where the certificate is located")
+	flagSet.StringVar(&o.GRPCAuthority, "tinkerbell-grpc-authority", "127.0.0.1:42113", "Link to tink-server grcp api")


Suggested change

flagSet.StringVar(&o.GRPCAuthority, "tinkerbell-grpc-authority", "127.0.0.1:42113", "Link to tink-server grcp api")

flagSet.StringVar(&o.GRPCAuthority, "tinkerbell-grpc-authority", "127.0.0.1:42113", "Link to tink-server grpc api")

Sigh I do this all the time

stolsma · 2021-01-25T18:40:09Z

@gianarb I like the client api changes! It is a good basis to extend and unifi the api later on! For me this is a lgtm, but that's not on me 😉

mmlb · 2021-01-25T18:47:53Z

protos/hardware/gen.go

@@ -0,0 +1,3 @@
+package hardware
+
+//go:generate moq -out mock.go . HardwareServiceClient HardwareService_AllClient


boots uses https://github.com/golang/mock/gomock we should probably pick one for both projects.

mmlb · 2021-01-25T18:56:51Z

Makefile

+protos/gen_mock:
+	go generate ./protos/**/*
+	goimports -w ./protos/**/mock.go


Would be great if these were proper deps for the necessary binaries so that they would always be up to date. Doesn't have to be now, I really want to see this PR done with :D. I can open up an issue when this PR is merged though

This commit bootstrap a single get command for the tink-cli. It does not matter the resource itself, it provides an abstraction that can be used by its own. Signed-off-by: Gianluca Arbezzano <[email protected]>

Signed-off-by: Gianluca Arbezzano <[email protected]>

This PR removes the use of `init()` functions from tink-cli/cmd/hardware and it uses a gRPC client that is not global. This is a required step to have a much more testable codebase Signed-off-by: Gianluca Arbezzano <[email protected]>

I was looking for something easy to use to generate moq structure from an interface. I discovered a great project from the awesome Mat called Moq https://github.com/matryer/moq. It generated structures starting from an interface following the strategy we use already for the db package. This commit contains a test case that prove how to use that mock for: tink hardware get Signed-off-by: Gianluca Arbezzano <[email protected]>

Signed-off-by: Gianluca Arbezzano <[email protected]>

Naming thing is pretty hard. I called the new client MetaClient because I didn't have a better name but Manny likes Full much more. Signed-off-by: Gianluca Arbezzano <[email protected]>

I used `-o/--output` as a flag to specify the format for the get command, mainly following the kubectl-way. The community prefers a more explicit `--format`, docker-like way. This commits leave `output` in favor of `format Signed-off-by: Gianluca Arbezzano <[email protected]>

This commit contains a couple of search and replace activities and some fixes for typos coming from code review Signed-off-by: Gianluca Arbezzano <[email protected]>

Signed-off-by: Gianluca Arbezzano <[email protected]>

This commit adds support for: $ tink get <resource> ID1 ID2 ID3 Supported resources: workflows, templates and hardware Signed-off-by: Gianluca Arbezzano <[email protected]>

Signed-off-by: Gianluca Arbezzano <[email protected]>

I thought it was a good idea to pass the FullClient to all the commands that interact with Tinkerbell server as part of the `New*` function. But it requires to attempt a connection to the tink-server very early and commands that do not need a client such as "docs", "help" were still requiring a running tink-server, and we do not want it. Signed-off-by: Gianluca Arbezzano <[email protected]>

gianarb added the breaking-change Denotes a PR that introduces potentially breaking changes that require user action. label Dec 28, 2020

gianarb requested review from mmlb, thebsdbox and gauravgahlot December 28, 2020 15:31

thebsdbox previously approved these changes Dec 31, 2020

View reviewed changes

cmd/tink-cli/cmd/workflow/list.go Outdated Show resolved Hide resolved

stolsma reviewed Dec 31, 2020

View reviewed changes

client/main.go Outdated Show resolved Hide resolved

stolsma reviewed Jan 4, 2021

View reviewed changes

client/main.go Show resolved Hide resolved

gianarb dismissed thebsdbox’s stale review via 0a270f7 January 4, 2021 10:30

mmlb suggested changes Jan 4, 2021

View reviewed changes

gianarb requested review from mmlb, thebsdbox and displague January 12, 2021 10:28

mmlb reviewed Jan 25, 2021

View reviewed changes

mmlb previously approved these changes Jan 25, 2021

View reviewed changes

Gianluca Arbezzano added 14 commits January 25, 2021 20:22

Bootstrap Get command for tink-cli

7bd1743

This commit bootstrap a single get command for the tink-cli. It does not matter the resource itself, it provides an abstraction that can be used by its own. Signed-off-by: Gianluca Arbezzano <[email protected]>

Skip tests that are currently failing because not well written

9f7d6fa

Signed-off-by: Gianluca Arbezzano <[email protected]>

Removed init function for cli template pkg

76793ee

Signed-off-by: Gianluca Arbezzano <[email protected]>

Remove init from cli/workflow

0cbb1f9

Signed-off-by: Gianluca Arbezzano <[email protected]>

Fixed test name for resource commands

4c1ed62

Signed-off-by: Gianluca Arbezzano <[email protected]>

Change MetaClient with FullClient

de79a38

Naming thing is pretty hard. I called the new client MetaClient because I didn't have a better name but Manny likes Full much more. Signed-off-by: Gianluca Arbezzano <[email protected]>

Rename a couple of variables and flags

a1d0390

This commit contains a couple of search and replace activities and some fixes for typos coming from code review Signed-off-by: Gianluca Arbezzano <[email protected]>

Create make target to generate protos mock

923c726

Signed-off-by: Gianluca Arbezzano <[email protected]>

Implement get by IDs for hardware, template and workflow

40dfbab

This commit adds support for: $ tink get <resource> ID1 ID2 ID3 Supported resources: workflows, templates and hardware Signed-off-by: Gianluca Arbezzano <[email protected]>

Move client setup as part of the command itself

dee255e

Signed-off-by: Gianluca Arbezzano <[email protected]>

gianarb dismissed mmlb’s stale review via 68a7185 January 25, 2021 19:22

gianarb requested review from mmlb and removed request for displague, thebsdbox, gauravgahlot and mmlb January 25, 2021 19:22

gianarb added the ready-to-merge Signal to Mergify to merge the PR. label Jan 25, 2021

mmlb approved these changes Jan 25, 2021

View reviewed changes

mergify bot merged commit b6c0901 into tinkerbell:master Jan 25, 2021

gianarb mentioned this pull request Jan 26, 2021

Tink CLI current state and the future is called consistency #405

Closed

This was referenced Jan 27, 2021

Reimplement a common delete tink-cli command #425

Closed

Implement tink-cli describe command #426

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bootstrap Get command for tink-cli #406

Bootstrap Get command for tink-cli #406

gianarb commented Dec 28, 2020 •

edited

Loading

codecov bot commented Dec 29, 2020 •

edited

Loading

thebsdbox commented Dec 31, 2020

thebsdbox left a comment

stolsma commented Dec 31, 2020 •

edited

Loading

gianarb commented Dec 31, 2020 •

edited

Loading

stolsma commented Jan 4, 2021

gianarb commented Jan 4, 2021

gianarb commented Jan 4, 2021 •

edited

Loading

displague commented Jan 4, 2021 •

edited

Loading

mmlb commented Jan 4, 2021

mmlb commented Jan 4, 2021

gianarb commented Jan 5, 2021

mmlb commented Jan 5, 2021

gianarb commented Jan 12, 2021

mmlb Jan 25, 2021

stolsma commented Jan 25, 2021

mmlb Jan 25, 2021

mmlb Jan 25, 2021

	flagSet.StringVar(&o.GRPCAuthority, "tinkerbell-grpc-authority", "127.0.0.1:42113", "Link to tink-server grcp api")
	flagSet.StringVar(&o.GRPCAuthority, "tinkerbell-grpc-authority", "127.0.0.1:42113", "Link to tink-server grpc api")

		@@ -0,0 +1,3 @@
		package hardware

		//go:generate moq -out mock.go . HardwareServiceClient HardwareService_AllClient

Bootstrap Get command for tink-cli #406

Bootstrap Get command for tink-cli #406

Conversation

gianarb commented Dec 28, 2020 • edited Loading

Description

Why is this needed

How Has This Been Tested?

How to migrate?

codecov bot commented Dec 29, 2020 • edited Loading

Codecov Report

thebsdbox commented Dec 31, 2020

thebsdbox left a comment

Choose a reason for hiding this comment

stolsma commented Dec 31, 2020 • edited Loading

gianarb commented Dec 31, 2020 • edited Loading

stolsma commented Jan 4, 2021

gianarb commented Jan 4, 2021

gianarb commented Jan 4, 2021 • edited Loading

displague commented Jan 4, 2021 • edited Loading

mmlb commented Jan 4, 2021

mmlb commented Jan 4, 2021

gianarb commented Jan 5, 2021

mmlb commented Jan 5, 2021

gianarb commented Jan 12, 2021

mmlb Jan 25, 2021

Choose a reason for hiding this comment

stolsma commented Jan 25, 2021

mmlb Jan 25, 2021

Choose a reason for hiding this comment

mmlb Jan 25, 2021

Choose a reason for hiding this comment

gianarb commented Dec 28, 2020 •

edited

Loading

codecov bot commented Dec 29, 2020 •

edited

Loading

stolsma commented Dec 31, 2020 •

edited

Loading

gianarb commented Dec 31, 2020 •

edited

Loading

gianarb commented Jan 4, 2021 •

edited

Loading

displague commented Jan 4, 2021 •

edited

Loading