Support fetching templates by name #349

kqdeng · 2020-10-26T14:07:26Z

Description

Adds the ability to query templates by name.
The ListTemplates method has been changed to accept a FilterRequest instead of just Empty.
The FilterRequest has a field filter that accepts a string used to match the names of templates. A wildcard * can be used to match a sequence of zero or more characters.
Passing an empty FilterRequest will return all templates.

Why is this needed

A user might have the following templates: hello-world, ubuntu-01, ubuntu-02, ubuntu-03, goodbye-world and wants to list only the linux templates. Before, they would only be able to either list all of the templates, or only one (and only if they already know the id).
With this PR, they can just call ListTemplates with the FilterRequest's filter field to ubuntu-* and it will return only ubuntu-01, ubuntu-02, and ubuntu-03.

Fixes: #352 / ENG-9455

How Has This Been Tested?

manually
TBD

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

Users using an older version of the tink-cli may experience errors. Best solution is to update the tink-server, tink-cli, and tink-worker to use the images built from this PR

Checklist:

I have:

updated the documentation and/or roadmap (if required)
added unit or e2e tests
provided instructions on how to upgrade

codecov · 2020-10-26T15:25:01Z

Codecov Report

Merging #349 into master will increase coverage by 0.95%.
The diff coverage is 19.14%.

@@            Coverage Diff             @@
##           master     #349      +/-   ##
==========================================
+ Coverage   23.79%   24.74%   +0.95%     
==========================================
  Files          14       14              
  Lines        1244     1277      +33     
==========================================
+ Hits          296      316      +20     
- Misses        928      940      +12     
- Partials       20       21       +1

Impacted Files	Coverage Δ
http-server/http_handlers.go	`6.19% <0.00%> (-0.49%)`	⬇️
grpc-server/template.go	`36.53% <44.44%> (+17.34%)`	⬆️
grpc-server/workflow.go	`21.00% <100.00%> (+0.39%)`	⬆️

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 322775a...775cdf7. Read the comment docs.

mmlb · 2020-10-26T20:22:39Z

I opened up #351 to fix the whitespace issue and avoid it in the future by making sure CI catches.

gianarb · 2020-10-26T22:16:22Z

Do we want somehow to formalize and make this filtering available for other resources? I am just trying to avoid an unfriendly proliferation of different filters that probably sounds useful short term but won't make usability nice in long term

kqdeng · 2020-10-27T15:57:24Z

@gianarb Can you elaborate? Are you suggesting we should also provide the option to filter by any attribute of a resource?

gianarb · 2020-10-27T17:42:46Z

@kdeng3849 yeah something like that.

I am wondering if we should build this feature in a way that will allow us to get filtering done for all the resources across fields in a consistent way. Without building something ad hoc for one case.

I think having the filtering done one by one will make the UX inconsistent and not great

gianarb

We need a rebase, @mmlb 's fault :P

mmlb · 2020-10-27T20:00:01Z

oh that merged already? :toot:

mmlb · 2020-10-29T21:18:28Z

grpc-server/template_test.go

+		want struct {
+			expectedError bool
+		}


can we make this something like:

Suggested change

want struct {

expectedError bool

}

err bool

instead? No need to define a struct for just one type and its easier to use too.

mmlb · 2020-10-29T21:29:40Z

@kdeng3849 yeah something like that.

I am wondering if we should build this feature in a way that will allow us to get filtering done for all the resources across fields in a consistent way. Without building something ad hoc for one case.

I think having the filtering done one by one will make the UX inconsistent and not great

Yeah I think maybe something like

...
rpc GetTemplate(GetRequest) returns (WorkflowTemplate)
...
message GetRequest {
  oneof get_oneof {
    string id = 1;
    string name = 2;
  }
}

we can leave the other oneof options for later if necessary. I think I'd rather see a pattern we can agree on that is extensible to avoid GetBy... for each proto type. Does this line up with what you're thinking @gianarb ?

gianarb · 2020-11-03T10:51:08Z

Yeah I think it sounds good @mmlb

DavidHwu · 2020-11-03T16:20:49Z

@mmlb & @gianarb this PR is blocking Central Eng (or @mattcburns ) from moving forward on a commitment integration deadline.
I've read your change requests... is this a must have or NTH (Nice To Have)?

While @mmlb comments I think is worthy... I believe this can be served in another ticket. Resulting in unblocking Matt Burns for this deadline.

gianarb

I am having some difficulties visualizing the grpc API right now but I don't want to hold more on this :)

mmlb · 2020-11-03T21:45:48Z

cmd/tink-cli/cmd/template/delete.go

+			grID := &template.GetRequest_Id{Id: arg}
+			req := template.GetRequest{GetBy: grID}


Can we do it like this instead please:

req := template.GetRequest{ GetBy: &template.GetRequest_Id{ Id: arg, }, }

No need to introduce a variable that we have to remember to ignore (grID) :D

mmlb · 2020-11-03T22:20:44Z

db/db.go

@@ -35,7 +36,7 @@ type hardware interface {

 type template interface {
 	CreateTemplate(ctx context.Context, name string, data string, id uuid.UUID) error
-	GetTemplate(ctx context.Context, id string) (string, string, error)
+	GetTemplate(ctx context.Context, attributes map[string]string) (string, string, error)


I think a struct will be better to use here.

mmlb · 2020-11-03T22:24:44Z

protos/template/template.proto

@@ -51,4 +51,9 @@ message CreateResponse {

 message GetRequest {
  string id = 1;
+  string name = 2;


this is in the wrong commit :D

mmlb · 2020-11-03T22:25:11Z

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

@@ -43,7 +43,7 @@ var listCmd = &cobra.Command{
 }

 func listTemplates(cmd *cobra.Command, t table.Writer) {
-	list, err := client.TemplateClient.ListTemplates(context.Background(), &template.FilterRequest{Filter: "%"})
+	list, err := client.TemplateClient.ListTemplates(context.Background(), &template.FilterRequest{Filter: "*"})


this commit should be squashed into the previous one

mmlb · 2020-11-03T22:27:00Z

grpc-server/template_test.go

@@ -2,6 +2,7 @@ package grpcserver



Please update the commit message to something like

Add tests for Get filtering

mmlb · 2020-11-03T22:29:42Z

grpc-server/workflow.go

@@ -270,15 +270,18 @@ func (s *server) ShowWorkflowEvents(req *workflow.GetRequest, stream workflow.Wo
 	return nil


I think adding createYaml to the commit message would help when looking through git history:

createYaml: use more descriptive argument names

Rename temp and tempData to templateID and templateData for better clarity.

mmlb · 2020-11-03T22:30:18Z

db/db.go

@@ -36,7 +36,7 @@ type hardware interface {

 type template interface {


agree with this, but should just be squashed in the commit that does it :D

mmlb · 2020-11-03T22:33:49Z

protos/template/template.proto

+message FilterRequest {
+  string filter = 1;
 }


I think it'd be great to have FilterRequest look like GetRequest so that we have one way to do filtering. This would also help to convey what field the filter is taking place on.

message FilterRequest { one_of filter_types { string name = 1; } }

Signed-off-by: Kelly Deng <[email protected]>

## Description Fetching a template by name now also returns the associated ID. ## Why is this needed A recent PR #349 added the functionality of fetching a template by name. The returned template is expected to include the ID, name, and template data. However, currently, it would only return the name and the template data. The returned ID is just an empty string. ## How Has This Been Tested? Manually ## How are existing users impacted? What migration steps/scripts do we need? ## Checklist: I have: - [ ] updated the documentation and/or roadmap (if required) - [x] added unit or e2e tests - [ ] provided instructions on how to upgrade

kqdeng force-pushed the ENG-9455-query-by-name-v2 branch 2 times, most recently from 7acfae7 to 2efcae7 Compare October 26, 2020 15:23

mmlb changed the title ~~ENG-9455 query by name~~ Support fetching templates by name Oct 26, 2020

gianarb approved these changes Oct 27, 2020

View reviewed changes

gianarb suggested changes Oct 27, 2020

View reviewed changes

kqdeng force-pushed the ENG-9455-query-by-name-v2 branch 5 times, most recently from 25ae6d9 to 8d15c9a Compare October 29, 2020 13:21

kqdeng requested review from gianarb and mmlb October 29, 2020 13:24

kqdeng force-pushed the ENG-9455-query-by-name-v2 branch from 8d15c9a to 7d790bc Compare October 29, 2020 14:42

kqdeng requested a review from splaspood October 29, 2020 18:13

mmlb suggested changes Oct 29, 2020

View reviewed changes

kqdeng force-pushed the ENG-9455-query-by-name-v2 branch from 7d790bc to 5dac292 Compare November 2, 2020 16:46

kqdeng requested a review from mmlb November 2, 2020 16:46

mattcburns previously approved these changes Nov 2, 2020

View reviewed changes

gianarb previously approved these changes Nov 3, 2020

View reviewed changes

mmlb suggested changes Nov 3, 2020

View reviewed changes

kqdeng dismissed gianarb’s stale review via 0fc74b6 November 3, 2020 22:49

kqdeng dismissed mattcburns’s stale review via 0fc74b6 November 3, 2020 22:49

kqdeng force-pushed the ENG-9455-query-by-name-v2 branch from 0fc74b6 to f69a0d0 Compare November 4, 2020 14:48

add filtering to ListTemplates

c186f90

Signed-off-by: Kelly Deng <[email protected]>

kqdeng force-pushed the ENG-9455-query-by-name-v2 branch from f69a0d0 to 3f8c387 Compare November 4, 2020 16:35

kqdeng added 4 commits November 4, 2020 14:10

allow only one of either id or name to be set within template.GetRequest

62f2549

Signed-off-by: Kelly Deng <[email protected]>

add http handler get by name

3a5a27d

Signed-off-by: Kelly Deng <[email protected]>

createYaml: use more descriptive argument names

e70fee9

Signed-off-by: Kelly Deng <[email protected]>

add tests for GetTemplate

775cdf7

Signed-off-by: Kelly Deng <[email protected]>

mmlb removed the request for review from splaspood November 4, 2020 19:10

mmlb added the ready-to-merge Signal to Mergify to merge the PR. label Nov 4, 2020

kqdeng force-pushed the ENG-9455-query-by-name-v2 branch from 3f8c387 to 775cdf7 Compare November 4, 2020 19:22

mmlb approved these changes Nov 4, 2020

View reviewed changes

mergify bot merged commit a94738e into tinkerbell:master Nov 4, 2020

kqdeng mentioned this pull request Nov 10, 2020

fix getting template by name not returning the associated id #362

Merged

3 tasks

mmlb removed the ready-to-merge Signal to Mergify to merge the PR. label Jan 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support fetching templates by name #349

Support fetching templates by name #349

kqdeng commented Oct 26, 2020 •

edited

Loading

codecov bot commented Oct 26, 2020 •

edited

Loading

mmlb commented Oct 26, 2020

gianarb commented Oct 26, 2020

kqdeng commented Oct 27, 2020

gianarb commented Oct 27, 2020

gianarb left a comment

mmlb commented Oct 27, 2020

mmlb Oct 29, 2020

mmlb commented Oct 29, 2020 •

edited

Loading

gianarb commented Nov 3, 2020

DavidHwu commented Nov 3, 2020 •

edited

Loading

gianarb left a comment

mmlb Nov 3, 2020

mmlb Nov 3, 2020

mmlb Nov 3, 2020

mmlb Nov 3, 2020

mmlb Nov 3, 2020

mmlb Nov 3, 2020

mmlb Nov 3, 2020

mmlb Nov 3, 2020

		grID := &template.GetRequest_Id{Id: arg}
		req := template.GetRequest{GetBy: grID}

		@@ -270,15 +270,18 @@ func (s server) ShowWorkflowEvents(req workflow.GetRequest, stream workflow.Wo
		return nil

		@@ -36,7 +36,7 @@ type hardware interface {

		type template interface {

Support fetching templates by name #349

Support fetching templates by name #349

Conversation

kqdeng commented Oct 26, 2020 • edited Loading

Description

Why is this needed

How Has This Been Tested?

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

Checklist:

codecov bot commented Oct 26, 2020 • edited Loading

Codecov Report

mmlb commented Oct 26, 2020

gianarb commented Oct 26, 2020

kqdeng commented Oct 27, 2020

gianarb commented Oct 27, 2020

gianarb left a comment

Choose a reason for hiding this comment

mmlb commented Oct 27, 2020

Choose a reason for hiding this comment

mmlb commented Oct 29, 2020 • edited Loading

gianarb commented Nov 3, 2020

DavidHwu commented Nov 3, 2020 • edited Loading

gianarb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kqdeng commented Oct 26, 2020 •

edited

Loading

codecov bot commented Oct 26, 2020 •

edited

Loading

mmlb commented Oct 29, 2020 •

edited

Loading

DavidHwu commented Nov 3, 2020 •

edited

Loading