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

Introduce a flag for auto publishing #886

Merged
merged 17 commits into from
Aug 9, 2016
Merged

Introduce a flag for auto publishing #886

merged 17 commits into from
Aug 9, 2016

Conversation

HuKeping
Copy link
Contributor

@HuKeping HuKeping commented Jul 29, 2016

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
Copy link

Can one of the admins verify this patch?

@endophage
Copy link
Contributor

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

@cyli
Copy link
Contributor

cyli commented Aug 1, 2016

jenkins, test this please

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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@riyazdf
Copy link
Contributor

riyazdf commented Aug 1, 2016

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
Copy link
Contributor Author

HuKeping commented Aug 2, 2016

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

if err != nil {
return err
}

Copy link
Contributor

Choose a reason for hiding this comment

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

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()

Copy link
Contributor

Choose a reason for hiding this comment

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

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>...)

Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

cyli commented Aug 3, 2016

jenkins, test this please

@@ -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.")
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@HuKeping HuKeping Aug 3, 2016

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

cyli commented Aug 3, 2016

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
Copy link
Contributor Author

HuKeping commented Aug 3, 2016

Tests ready.

gun string = "MistsOfPandaria"
gunNoPublish string = "Legion"

// This might be changed via the implmentation, please be care.
Copy link
Contributor

@riyazdf riyazdf Aug 4, 2016

Choose a reason for hiding this comment

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

typo: ...implementation, please be careful

@riyazdf
Copy link
Contributor

riyazdf commented Aug 4, 2016

@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
Copy link
Contributor Author

HuKeping commented Aug 4, 2016

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

@HuKeping
Copy link
Contributor Author

HuKeping commented Aug 4, 2016

Covers all the commands listed in #885

@riyazdf
Copy link
Contributor

riyazdf commented Aug 4, 2016

jenkins, test this please

// 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
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: expect

@cyli
Copy link
Contributor

cyli commented Aug 4, 2016

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

@HuKeping
Copy link
Contributor Author

HuKeping commented Aug 5, 2016

notary addhash added

@riyazdf
Copy link
Contributor

riyazdf commented Aug 5, 2016

jenkins, test this please

@HuKeping
Copy link
Contributor Author

HuKeping commented Aug 5, 2016

Finally all green lol

// 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))
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

require.NoError(t, err)
require.True(t, strings.Contains(string(output), target1))

// remove target
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed in commit Refactor and clean up test

@cyli
Copy link
Contributor

cyli commented Aug 5, 2016

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

@riyazdf
Copy link
Contributor

riyazdf commented Aug 8, 2016

jenkins, test this please.

@riyazdf
Copy link
Contributor

riyazdf commented Aug 8, 2016

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

@endophage endophage changed the title Introduce a flag for auto pubilshing Introduce a flag for auto publishing Aug 8, 2016
@@ -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())
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for cleaning these up!

@riyazdf
Copy link
Contributor

riyazdf commented Aug 9, 2016

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

HuKeping added 17 commits August 9, 2016 11:19
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]>
It might be used by wherever we want to end with an auto publish.

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]>
@HuKeping
Copy link
Contributor Author

HuKeping commented Aug 9, 2016

rebased

@cyli
Copy link
Contributor

cyli commented Aug 9, 2016

jenkins test this please

@cyli cyli merged commit f76c23e into notaryproject:master Aug 9, 2016
@HuKeping HuKeping deleted the autopublish branch August 10, 2016 00:54
@HuKeping
Copy link
Contributor Author

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

@endophage
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants