Introduce a flag for auto publishing #886

HuKeping · 2016-07-29T08:43:23Z

Closes #885

Add flag "-p", "--publish" to "notary add", with this flag being set,
notary will attemp to publish after adding the change.

Signed-off-by: Hu Keping [email protected]

docker-jenkins · 2016-07-29T08:43:24Z

Can one of the admins verify this patch?

endophage · 2016-08-01T21:14:46Z

Looks like a great start. Would like to see this option take effect on every command that currently generates a changelist item.

cyli · 2016-08-01T21:20:51Z

jenkins, test this please

riyazdf · 2016-08-01T22:05:27Z

cmd/notary/tuf.go

+	cmd.Printf("Addition of target \"%s\" to repository \"%s\" staged for next publish.\n", targetName, gun)
+
+	// Auto publish the local trusted collection to remote.
+	if t.autoPublish == true {


nit: can we refactor this line to if t.autoPublish {?

riyazdf · 2016-08-01T22:57:47Z

I agree with @endophage's comment - it would be great if this PR could add the flag to all commands listed in #885 and also if we could add some simple tests

HuKeping · 2016-08-02T01:51:53Z

Please take a look if this change is OK, I'll add the others if so.

riyazdf · 2016-08-02T17:51:03Z

cmd/notary/tuf.go

+		if err != nil {
+			return err
+		}
+


this looks good, the only thing I would add is to have a print similar to how we have one in tufPublish, something like:

... cmd.Println("Auto-publishing changes to", gun) return nRepo.Publish()

this is also quite a few lines of code that I think will be identical everywhere we want to do this so I'd wrap the whole thing, including the if t.autoPublish in its own function and then anywhere we want to end with an auto publish we can just do something like return maybeAutoPublish(t.autoPublish, nRepo, <other args>...)

Also, some of the commands we'd want to autopublish are in other places (i.e. notary delegation add/remove) so it would be good to work out a more accessible place for the flag.

cyli · 2016-08-03T04:52:03Z

jenkins, test this please

cyli · 2016-08-03T05:10:13Z

cmd/notary/tuf.go

@@ -123,6 +125,7 @@ func (t *tufCommander) AddToCommand(cmd *cobra.Command) {

 	cmdTUFAdd := cmdTUFAddTemplate.ToCommand(t.tufAdd)
 	cmdTUFAdd.Flags().StringSliceVarP(&t.roles, "roles", "r", nil, "Delegation roles to add this target to")
+	cmdTUFAdd.Flags().BoolVarP(&t.autoPublish, "publish", "p", false, "Automatically attempt to publish after staging the change. Will also publish existing staged changes.")


Since this is a flag that is going to be added to multiple commands, I'm wondering if we should make the help text be a constant?

We can benefit from the constant especially when we want to modify the help text in the future, we may missing some if we don't use constant. But also I'm wondering if it was a good idea to use a constant for a command help text, I haven't found an example.

I agree with @cyli - also for when we implement the delegation add and delegation remove auto-publish flags, I think the text will be the same, and we've used constants for similar important text like tufRootKeyGenerationWarning

cyli · 2016-08-03T05:23:35Z

Thank you for your work on this @HuKeping! This looks much cleaner, and am looking forward to having this flag on the offline commands! :)

HuKeping · 2016-08-03T10:00:09Z

Tests ready.

riyazdf · 2016-08-04T02:33:50Z

cmd/notary/integration_test.go

+		gun          string = "MistsOfPandaria"
+		gunNoPublish string = "Legion"
+
+		// This might be changed via the implmentation, please be care.


typo: ...implementation, please be careful

riyazdf · 2016-08-04T02:40:02Z

@HuKeping would you be alright with also including notary delegation add -p and notary delegation remove -p in this PR so we get all of the auto-publish flags in simultaneously? It would iron out the UX for all of our printing and flag information, and would also ensure the design of the auto-publish helper functions are solid.

If you would prefer a separate PR that's fine too, but those are the only two auto-publish commands left and you've made some really awesome progress in this PR already :)

HuKeping · 2016-08-04T04:07:55Z

@riyazdf It should be in one PR, will add the delegation case

HuKeping · 2016-08-04T09:38:55Z

Covers all the commands listed in #885

riyazdf · 2016-08-04T16:25:47Z

jenkins, test this please

riyazdf · 2016-08-04T16:28:11Z

cmd/notary/integration_test.go

+	// init repo with auto publish being enabled
+	_, err = runCommand(t, tempDir, "-s", server.URL, "init", "-p", gun)
+	require.NoError(t, err)
+	// list repo - exepect empty list


typo: expect

cyli · 2016-08-04T21:57:06Z

Should we also add a -p flag for the adding by hash command?

HuKeping · 2016-08-05T08:07:11Z

notary addhash added

riyazdf · 2016-08-05T16:24:26Z

jenkins, test this please

HuKeping · 2016-08-05T18:13:18Z

Finally all green lol

riyazdf · 2016-08-05T18:38:36Z

cmd/notary/integration_test.go

+	// so the target and target2 should be in the list.
+	output, err = runCommand(t, tempDir, "-s", server.URL, "list", gun)
+	require.NoError(t, err)
+	require.True(t, strings.Contains(output, target))


super non-blocking nit: these string contains can be simplified to require.Contains(t, output, target)

Fixed in commit Use require.Contains other than require.True when neccessary, also fix the others test in this file

cyli · 2016-08-05T20:29:14Z

cmd/notary/integration_test.go

+	require.NoError(t, err)
+	require.True(t, strings.Contains(string(output), target1))
+
+	// remove target


Totally non-blocking - since this test case is just testing the autopublish functionality, the removal and testing different types of hashes may not be as necessary.

addressed in commit Refactor and clean up test

cyli · 2016-08-05T20:30:08Z

Thanks for adding these @HuKeping! Aside from @riyazdf's comment about testing malformed URLs (#886 (comment)), this LGTM!

riyazdf · 2016-08-08T16:39:54Z

jenkins, test this please.

riyazdf · 2016-08-08T17:01:29Z

LGTM. Thank you @HuKeping for your continued work on this feature!

endophage · 2016-08-08T17:10:19Z

cmd/notary/integration_test.go

@@ -119,7 +120,7 @@ func TestInitWithRootKey(t *testing.T) {
 	// check that the root key used for init is the one listed as root key
 	output, err := runCommand(t, tempDir, "key", "list")
 	require.NoError(t, err)
-	require.True(t, strings.Contains(output, data.PublicKeyFromPrivate(privKey).ID()))
+	require.Contains(t, output, data.PublicKeyFromPrivate(privKey).ID())


Thanks for cleaning these up!

riyazdf · 2016-08-09T00:07:29Z

@HuKeping - sorry for the inconvenience but could you please rebase this branch?

Add flag "-p", "--publish" to "notary add", with this flag being set, notary will attemp to publish after adding the change. Signed-off-by: Hu Keping <[email protected]>

Signed-off-by: Hu Keping <[email protected]>

It might be used by wherever we want to end with an auto publish. Signed-off-by: Hu Keping <[email protected]>

Signed-off-by: Hu Keping <[email protected]>

Per riyaz's suggestions - using maybeAutoPublish would be more clear that we may not actully publish. Signed-off-by: Hu Keping <[email protected]>

Signed-off-by: Hu Keping <[email protected]>

And also fix a golint warning. Signed-off-by: Hu Keping <[email protected]>

Signed-off-by: Hu Keping <[email protected]>

HuKeping · 2016-08-09T04:53:39Z

rebased

cyli · 2016-08-09T06:13:52Z

jenkins test this please

HuKeping · 2016-08-10T01:16:49Z

Great to see this has been merged! Thanks for all your continued review on this! 🎉

endophage · 2016-08-11T00:28:28Z

@HuKeping thanks for picking it up! This is a really useful addition we're excited to see added.

GordonTheTurtle added status/0-triage labels Jul 29, 2016

riyazdf reviewed Aug 1, 2016
View reviewed changes

riyazdf reviewed Aug 2, 2016
View reviewed changes

cyli reviewed Aug 3, 2016
View reviewed changes

riyazdf reviewed Aug 4, 2016
View reviewed changes

riyazdf reviewed Aug 5, 2016
View reviewed changes

cyli reviewed Aug 5, 2016
View reviewed changes

endophage changed the title ~~Introduce a flag for auto pubilshing~~ Introduce a flag for auto publishing Aug 8, 2016

endophage reviewed Aug 8, 2016
View reviewed changes

HuKeping added 17 commits August 9, 2016 11:19

Introduce a flag for auto pubilshing

8a84d29

Add flag "-p", "--publish" to "notary add", with this flag being set, notary will attemp to publish after adding the change. Signed-off-by: Hu Keping <[email protected]>

Address riyazdf's comments

c5cecf0

Signed-off-by: Hu Keping <[email protected]>

Create a common function for auto publish

bd78254

It might be used by wherever we want to end with an auto publish. Signed-off-by: Hu Keping <[email protected]>

Introduce auto publishing for notary init

65cc13e

Signed-off-by: Hu Keping <[email protected]>

Introduce auto publish for notary remove

a232853

Signed-off-by: Hu Keping <[email protected]>

Add integration test for auto publishing

df89d32

Signed-off-by: Hu Keping <[email protected]>

Rename function name

57beb01

Per riyaz's suggestions - using maybeAutoPublish would be more clear that we may not actully publish. Signed-off-by: Hu Keping <[email protected]>

Generate a constant for the help test of auto publish

3bf2441

Signed-off-by: Hu Keping <[email protected]>

Introduce auto publish for notary delegation add

c518c71

Signed-off-by: Hu Keping <[email protected]>

Fix golint

35f88eb

Signed-off-by: Hu Keping <[email protected]>

Introduce auto publish for notary delegation remove

ba98365

Signed-off-by: Hu Keping <[email protected]>

Add test for notary delegation add/remove with auto publish

a451910

And also fix a golint warning. Signed-off-by: Hu Keping <[email protected]>

Enhance test for auto publish

63057a3

Signed-off-by: Hu Keping <[email protected]>

Introduce auto publish for notary addhash

e1e58ae

Signed-off-by: Hu Keping <[email protected]>

Add test for notary addhash with auto publish

bcfee5f

Signed-off-by: Hu Keping <[email protected]>

Use require.Contains other than require.True when neccessary

82dffe0

Signed-off-by: Hu Keping <[email protected]>

Refactor and clean up test

6c386a6

Signed-off-by: Hu Keping <[email protected]>

cyli merged commit f76c23e into notaryproject:master Aug 9, 2016

HuKeping deleted the autopublish branch August 10, 2016 00:54

Introduce a flag for auto publishing #886

Introduce a flag for auto publishing #886

Conversation

HuKeping commented Jul 29, 2016 • edited Loading

docker-jenkins commented Jul 29, 2016

endophage commented Aug 1, 2016

cyli commented Aug 1, 2016

Choose a reason for hiding this comment

riyazdf commented Aug 1, 2016

HuKeping commented Aug 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyli commented Aug 3, 2016

Choose a reason for hiding this comment

HuKeping Aug 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyli commented Aug 3, 2016

HuKeping commented Aug 3, 2016

riyazdf Aug 4, 2016 • edited Loading

Choose a reason for hiding this comment

riyazdf commented Aug 4, 2016

HuKeping commented Aug 4, 2016

HuKeping commented Aug 4, 2016

riyazdf commented Aug 4, 2016

Choose a reason for hiding this comment

cyli commented Aug 4, 2016

HuKeping commented Aug 5, 2016 • edited Loading

riyazdf commented Aug 5, 2016

HuKeping commented Aug 5, 2016

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

cyli commented Aug 5, 2016 • edited Loading

riyazdf commented Aug 8, 2016

riyazdf commented Aug 8, 2016

Choose a reason for hiding this comment

riyazdf commented Aug 9, 2016

HuKeping commented Aug 9, 2016

cyli commented Aug 9, 2016

HuKeping commented Aug 10, 2016

endophage commented Aug 11, 2016

HuKeping commented Jul 29, 2016 •

edited

Loading

HuKeping Aug 3, 2016 •

edited

Loading

riyazdf Aug 4, 2016 •

edited

Loading

HuKeping commented Aug 5, 2016 •

edited

Loading

cyli commented Aug 5, 2016 •

edited

Loading