Fix #377: Removing name flag from the tink template create/update command #385

parauliya · 2020-12-11T08:59:10Z

Description

This PR will remove the name flag from the tink template create/update command in tink-cli.

Why is this needed

Fixes: #377

How Has This Been Tested?

This has been tested manually over the vagrant setup.

How to migrate?

Current Behaviour:
At present, the tink template create -n <name> allows users to provide a name to a workflow template. A user can also update this name using the tink template update -n <new-name> command.

The workflow template also has the name field. So, a workflow is getting itss name from two different and places and this can be confusing to users.

version: "0.1"
name: hello_world_workflow
global_timeout: 6
tasks:
  - name: "hello world"
    worker: "{{.device_1}}"
    actions:
      - name: "hello_world"
        image: hello-world
        timeout: 60

New Behaviour:
With this PR, tink template create and tink template update CLI will not accept a name for the template to be created or updated. Instead, the template name will be picked from the name field in the template definition.

The existing users need to run the tink template update <id> --file <template-file> command. This will update the name of each template as per the value of name field in the template definition.

Checklist:

I have:

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

codecov · 2020-12-11T09:03:52Z

Codecov Report

Merging #385 (fe503ee) into master (745c2d0) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #385   +/-   ##
=======================================
  Coverage   33.08%   33.08%           
=======================================
  Files          24       24           
  Lines        2170     2170           
=======================================
  Hits          718      718           
  Misses       1376     1376           
  Partials       76       76

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 745c2d0...fe503ee. Read the comment docs.

gianarb · 2020-12-11T09:45:34Z

This is a BC break and I think we should take this opportunity to carefully review our UX.

This is what I mean in practice: tink hardware push uses -f to require the location for a file, here we use -p for the same thing. Should we move it to -f to gain some sort of consistency?

gauravgahlot · 2020-12-11T10:12:18Z

This is a BC break and I think we should take this opportunity to carefully review our UX.

This is what I mean in practice: tink hardware push uses -f to require the location for a file, here we use -p for the same thing. Should we move it to -f to gain some sort of consistency?

tink hardware push does not have a shorthand for file path and uses --file. -f is used for the facility.

Usage:
  tink hardware push [flags]

Examples:
cat /tmp/data.json | tink hardware push
tink hardware push --file /tmp/data.json

Flags:
      --file string   hardware data file
  -h, --help          help for push

Global Flags:
  -f, --facility string   used to build grpc and http urls

either pipe the data or provide the required '--file' flag

That's the reason why we had to use -p for the file path.

gianarb · 2020-12-11T10:14:25Z

Thank you for clarifying @gauravgahlot but my point is the same:

hardware use --file when asking for the location for a file.
template uses -p for the same reason.

Should we use the same one? 👍 👎

gauravgahlot · 2020-12-11T10:18:54Z

Thank you for clarifying @gauravgahlot but my point is the same:

hardware use --file when asking for the location for a file.
template uses -p for the same reason.

Should we use the same one?

Oh yes totally. Initially, we had thought of removing facility altogether and use -f for the file path. But I'm assuming the facility is required in the Packet environment. 🤔

gianarb · 2020-12-11T10:20:30Z

But I'm assuming the facility is required in the Packet environment

Never say to me that something is required for ONE single company.. :) but yeah for now I think we should use --file and leave the facility, probably when @mmlb will start removing Packet or dead code from all the repo -f/--facility will go away, but out of scope for now.

gauravgahlot · 2020-12-11T10:33:40Z

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

 	filePath     string
 	templateName string
 )

+type TemplateName struct {


Should not be an exported type.

gauravgahlot · 2020-12-11T10:35:43Z

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

 // createCmd represents the create subcommand for template command
 var createCmd = &cobra.Command{
 	Use:   "create",
 	Short: "create a workflow template ",
 	Example: `tink template create [flags]
-cat /tmp/example.tmpl | tink template create -n example`,
+cat /tmp/example.tmpl | tink template create `,


Can we please add more examples in the Long description?
For reference, you can check tink events watch.

gauravgahlot · 2020-12-11T10:36:36Z

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

+		templateName = templ.Name
+		return nil
+	}
+	return errors.New("Template does not have `name` field which is mandatory")


Please move all the error messages to a const block and ensure they all start with a lower case.

mmlb · 2020-12-11T15:20:51Z

But I'm assuming the facility is required in the Packet environment

Never say to me that something is required for ONE single company.. :) but yeah for now I think we should use --file and leave the facility, probably when @mmlb will start removing Packet or dead code from all the repo -f/--facility will go away, but out of scope for now.

Facility is only really used for the cert generation stuff, something I'd actually like to get rid of. Let me open up an issue for that (#389).

gianarb

Great, @parauliya do you mind changing -p with --file and we are good? Thanks

mmlb · 2020-12-14T18:06:54Z

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

+	var templ TemplateName
+	err := yaml.Unmarshal([]byte(data), &templ)
+	if err != nil {
+		return err
+	}
+	if templ.Name != "" {
+		templateName = templ.Name
+		return nil
+	}
+	return errors.New("Template does not have `name` field which is mandatory")


This should use github.com/tinkerbell/tink/workflow.Parse instead.

gianarb

This is a bc break, according with the proposal 0011 https://github.com/tinkerbell/proposals/blob/master/proposals/0011/README.md can you add a chapter in the PR description:

How to migrate

https://github.com/tinkerbell/proposals/blob/master/proposals/0011/README.md#how-to-migrate

Explaining what is not working anymore and how to do the same in the new way, thanks

gianarb · 2020-12-15T09:06:03Z

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

@@ -69,16 +74,23 @@ func readAll(reader io.Reader) []byte {
 func addFlags() {
 	flags := createCmd.PersistentFlags()
 	flags.StringVarP(&filePath, "path", "p", "", "path to the template file")


Please change this with --file to unify with hardware push

gauravgahlot · 2021-01-04T17:01:25Z

@mmlb Please let me know once you are back, and I will update the branch (one last time). 😄

mmlb · 2021-01-04T20:09:57Z

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

+	file     = "file"
+	filePath string


I don't quite see the reason for file global var when its not used in the flags.StringVarP call. It should either be named a little more descriptively (filePathArgName?) and used there too or we just hard code use of "file" every where.

I'm not a fan of the "variable for the variable name" approach...

mmlb · 2021-01-04T20:10:11Z

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

 	PreRunE: func(c *cobra.Command, args []string) error {
 		if !isInputFromPipe() {
-			path, _ := c.Flags().GetString(fPath)
+			path, _ := c.Flags().GetString(file)


path isn't ever being used right?

mmlb · 2021-01-04T20:30:44Z

cmd/tink-cli/cmd/template_test.go

@@ -61,8 +61,8 @@ func Test_templateCmd(t *testing.T) {
 				if err := root.Execute(); err != nil {
 					t.Error(err)
 				}
-				if !strings.Contains(out.String(), "create a workflow template") {
-					t.Error("expected output should include create a workflow template")
+				if !strings.Contains(out.String(), "Create template using the --file flag") {


can be DRY'd by doing:

want := "Create template using the --file flag" if !strings.Contains(out.String(), want) { t.Error("unexpected output, looking for %q as a substring in %q", want, out.String()) }

or similar

mmlb · 2021-01-04T20:30:53Z

cmd/tink-cli/cmd/template_test.go

@@ -112,8 +112,8 @@ func Test_templateCmd(t *testing.T) {
 				if err := root.Execute(); err != nil {
 					t.Error(err)
 				}
-				if !strings.Contains(out.String(), "update a template") {
-					t.Error("expected output should include update a template")
+				if !strings.Contains(out.String(), "Update an existing template") {


…te/update' command in tink-cli Signed-off-by: parauliya <[email protected]>

Signed-off-by: Gaurav Gahlot <[email protected]>

Few weeks ago we decided to use --file for template creation: #385 But a few days days ago probably during a rebase I broke that behavior and left the template create command in a broken state 76793ee#diff-f5b9b4afa42ee036d5b6836337e9d6273de8c341267f7ea75eddc561f0ad6441 As detected by Dan #445 This commit should fix the issue, and it will require a documentation update as soon as we release a new version of sandbox.

Few weeks ago we decided to use --file for template creation: #385 But a few days days ago probably during a rebase I broke that behavior and left the template create command in a broken state 76793ee#diff-f5b9b4afa42ee036d5b6836337e9d6273de8c341267f7ea75eddc561f0ad6441 As detected by Dan #445 This commit should fix the issue, and it will require a documentation update as soon as we release a new version of sandbox. Signed-off-by: Gianluca Arbezzano <[email protected]>

A few weeks ago we decided to use --file for template creation: #385 But a few days ago probably during a rebase, I broke that behavior and left the template to create command in a broken state 76793ee#diff-f5b9b4afa42ee036d5b6836337e9d6273de8c341267f7ea75eddc561f0ad6441 As detected by Dan #445 This commit should fix the issue, and it will require a documentation update as soon as we release a new version of the sandbox. Fixed #445

parauliya requested review from gianarb and gauravgahlot December 11, 2020 08:59

parauliya self-assigned this Dec 11, 2020

parauliya force-pushed the Fix_377 branch from afde2c4 to 764f25b Compare December 11, 2020 09:00

parauliya mentioned this pull request Dec 11, 2020

Docs updated for the Fix of issue #377 tinkerbell/tinkerbell-docs#46

Closed

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

gauravgahlot suggested changes Dec 11, 2020

View reviewed changes

gianarb changed the title ~~Fix #377: Removing name flag from the 'tink template create/update' command~~ Fix #377: Removing name flag from the tink template create/update command Dec 11, 2020

gianarb requested a review from mmlb December 11, 2020 13:08

gianarb suggested changes Dec 11, 2020

View reviewed changes

mmlb approved these changes Dec 14, 2020

View reviewed changes

mmlb suggested changes Dec 14, 2020

View reviewed changes

gianarb suggested changes Dec 15, 2020

View reviewed changes

gauravgahlot force-pushed the Fix_377 branch 2 times, most recently from 28250b4 to b0a5672 Compare December 23, 2020 05:08

gauravgahlot requested review from mmlb, gianarb and gauravgahlot and removed request for gauravgahlot December 23, 2020 11:03

gauravgahlot previously approved these changes Dec 23, 2020

View reviewed changes

gauravgahlot self-assigned this Dec 23, 2020

gianarb previously approved these changes Jan 4, 2021

View reviewed changes

gauravgahlot dismissed stale reviews from gianarb and themself via 0589d44 January 4, 2021 08:42

gauravgahlot force-pushed the Fix_377 branch from 1138340 to 0589d44 Compare January 4, 2021 08:42

mmlb suggested changes Jan 4, 2021

View reviewed changes

gauravgahlot force-pushed the Fix_377 branch from 0589d44 to f0dbd56 Compare January 5, 2021 05:13

gauravgahlot requested review from mmlb, gauravgahlot and gianarb January 5, 2021 05:20

gauravgahlot force-pushed the Fix_377 branch from f0dbd56 to 0f1a7ff Compare January 12, 2021 08:56

gauravgahlot previously approved these changes Jan 12, 2021

View reviewed changes

parauliya and others added 4 commits January 21, 2021 14:42

Fix tinkerbell#377: Removing name flag from the 'tink template crea…

d667759

…te/update' command in tink-cli Signed-off-by: parauliya <[email protected]>

review fixes

7b90413

Signed-off-by: Gaurav Gahlot <[email protected]>

updated command description

7d58755

Signed-off-by: Gaurav Gahlot <[email protected]>

fixed failing tests

fe503ee

Signed-off-by: Gaurav Gahlot <[email protected]>

gauravgahlot dismissed their stale review via fe503ee January 21, 2021 09:12

gauravgahlot force-pushed the Fix_377 branch from 0f1a7ff to fe503ee Compare January 21, 2021 09:12

gianarb approved these changes Jan 21, 2021

View reviewed changes

gauravgahlot self-requested a review January 21, 2021 09:31

gauravgahlot approved these changes Jan 21, 2021

View reviewed changes

mmlb approved these changes Jan 21, 2021

View reviewed changes

mergify bot merged commit 26abf73 into tinkerbell:master Jan 21, 2021

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

parauliya deleted the Fix_377 branch February 25, 2021 07:51

gianarb mentioned this pull request Mar 3, 2021

Tink CLI Template create should use --file instead of -p #446

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #377: Removing name flag from the tink template create/update command #385

Fix #377: Removing name flag from the tink template create/update command #385

parauliya commented Dec 11, 2020 •

edited by gauravgahlot

Loading

codecov bot commented Dec 11, 2020 •

edited

Loading

gianarb commented Dec 11, 2020 •

edited

Loading

gauravgahlot commented Dec 11, 2020

gianarb commented Dec 11, 2020

gauravgahlot commented Dec 11, 2020

gianarb commented Dec 11, 2020

gauravgahlot Dec 11, 2020

gauravgahlot Dec 11, 2020

gauravgahlot Dec 11, 2020 •

edited

Loading

mmlb commented Dec 11, 2020

gianarb left a comment

mmlb Dec 14, 2020

gianarb left a comment

gianarb Dec 15, 2020

gauravgahlot commented Jan 4, 2021

mmlb Jan 4, 2021

mmlb Jan 4, 2021

mmlb Jan 4, 2021

mmlb Jan 4, 2021

Fix #377: Removing name flag from the tink template create/update command #385

Fix #377: Removing name flag from the tink template create/update command #385

Conversation

parauliya commented Dec 11, 2020 • edited by gauravgahlot Loading

Description

Why is this needed

How Has This Been Tested?

How to migrate?

Checklist:

codecov bot commented Dec 11, 2020 • edited Loading

Codecov Report

gianarb commented Dec 11, 2020 • edited Loading

gauravgahlot commented Dec 11, 2020

gianarb commented Dec 11, 2020

gauravgahlot commented Dec 11, 2020

gianarb commented Dec 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gauravgahlot Dec 11, 2020 • edited Loading

Choose a reason for hiding this comment

mmlb commented Dec 11, 2020

gianarb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gianarb left a comment

Choose a reason for hiding this comment

How to migrate

Choose a reason for hiding this comment

gauravgahlot commented Jan 4, 2021

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

parauliya commented Dec 11, 2020 •

edited by gauravgahlot

Loading

codecov bot commented Dec 11, 2020 •

edited

Loading

gianarb commented Dec 11, 2020 •

edited

Loading

gauravgahlot Dec 11, 2020 •

edited

Loading