Updates to publishing: flags, invocation image tagging by vdice · Pull Request #1405 · getporter/porter

vdice · 2021-01-08T17:14:47Z

What does this change

Updates porter publish per the manifest update epic in #1151:

Effectively renames the former tag flag to reference for specifying a full bundle reference for publishing
- Note: as this flag is part of the BundlePullOptions, the name changes for other commands as well, e.g. porter install --reference mybuns:v0.1.0
Updates the tag flag to just designate the Docker tag portion of the published bundle
Adds the registry flag to designate the Docker registry (and org/subdomain) portion of the published bundle
Updates logic in publish to tag (as in docker tag) an updated invocation image (say, if a new registry value is provided on publish), so that the image is found for publishing. Previously, a new invocation image name would trigger a full new re-build.

Ref #1335

What issue does it fix

Closes #1335
Closes #1151

Notes for the reviewer

Checklist

Unit Tests
Documentation
Schema (porter.yaml)

If this is your first pull request, please add your name to the bottom of our Contributors list. Thank you for making Porter better! 🙇‍♀️

Signed-off-by: Vaughn Dice <vadice@microsoft.com>

carolynvs · 2021-01-08T18:16:12Z

Looks like the stamp constant changed for one of the tests, once that's updated the tests should pass.

Signed-off-by: Vaughn Dice <vadice@microsoft.com>

carolynvs

Looks great! Thank you for the massive rename and logic switcharoo. Just had some questions to make sure I fully understand the change and then we are good to go.

carolynvs · 2021-01-12T16:23:56Z

 	data = append(data, v...)

-	// (for instance, during publish), so add this to the data
-	data = append(data, c.Manifest.Image...)


Sorry I know we talked about this but I've gotten fuzzy over the holidays. Can you remind me why we can remove this? I am just wondering if the info is now in the generate manifest, or if we can't put it in anymore because the flag may have changed the value?

I/we had added this previously to automatically trigger a rebuild whenever a bundle's image was updated. (Say, when a new registry or tag value is supplied on publish.) However, in this PR, instead of rebuilding the bundle, we re-tag images/bundle references and use these for publishing.

As noted elsewhere, it does mean that the actual contents (bundle.json/porter.yaml) included in the installer/invocation image may have different registry/reference values from the final published versions. For instance, the built bundle might have registry: myregistry.com but if I publish the bundle via porter publish --registry localhost:5000, the published invocation image (localhost:5000/mybuns:v0.1.0) will have a porter.yaml file with the original registry: myregistry.com field.

Thanks for clearing that up. I know some users want to be able to publish to alternate locations, either because they moved it across an airgap or something else, and have the bundle not contain the original information. One workaround in that case would be to use non-sensitive information when building the bundle and then always set a --registry, etc during publish.

carolynvs · 2021-01-12T16:25:48Z

@@ -47,6 +47,10 @@ type Manifest struct {
 	// and isn't meant to be user-specified
 	BundleTag string `yaml:"-"`


Do we use this field, BundleTag, to hold the final computed bundle reference? Or can we remove it?

I see it used in code but it's super confusing and I think a relic from a previous config setting we removed. Now may be a real good time to get rid of it completely (in a follow-up).

Do we use this field, BundleTag, to hold the final computed bundle reference? Or can we remove it?

Not anymore - now we use Reference to track the bundle reference. Indeed, BundleTag is ripe for removal. I can create an issue.

carolynvs · 2021-01-12T16:28:12Z

-		if err := p.LoadManifestFrom(build.LOCAL_MANIFEST); err != nil {
-			return err
-		}
+	if err := p.LoadManifestFrom(build.LOCAL_MANIFEST); err != nil {


What is the flow when someone does publish --reference. Do we do a full rebuild, or do we only retag? I'm wondering why it's safe now to unconditionally load the generated manifest.

I'm not you asking to change anything, just making sure I understand the new workflow with these tags.

Good question. Now, when a --reference override is supplied, we only retag (invocation image and final bundle reference). This does mean the bundle.json and porter.yaml files in the invocation image will be from the unchanged internal manifest.

Can you document this new "re-tag not rebuild" behavior in the LongDescription of the porter publish command?

👍 Added.

carolynvs · 2021-01-12T16:38:27Z

  porter bundle publish --file myapp/porter.yaml
-  porter bundle publish --archive /tmp/mybuns.tgz --tag myrepo/my-buns:0.1.0
+  porter bundle publish --archive /tmp/mybuns.tgz --reference myrepo/my-buns:0.1.0
+  porter bundle publish --tag latest


It's going to be a rough transition because we changed the meaning of this flag. How about in a follow-up we add validation for the publish options that checks if they are using --tag to specify the entire reference and error out explaining that the flags have changed?

We can identify that --tag contains a reference by checking for :/@, neither of which are allowed in a docker tag.

Good point. I almost think it's warranted to delay this PR until another commit including this logic is pushed. WDYT? I can work on this.

If you are up for it, that's great.

Ok, I added logic to error accordingly for the tag option on publish.

Do we want to do something similar for the other commands that previously also had a tag opt (courtesy BundlePullOptions)? As it stands, users who try to invoke porter install --tag mybuns:v0.1.0 will just see a standard Error: unknown flag: --tag after this is merged (and need to check the docs to see --reference is the new opt). Naturally, in the release notes, we can note this breaking cli opt change... not sure if we should also bake in a deprecation warning or error.

Yeah, we could add back a Tag field and flag definitions and then check if it was populated and give a better error message. It would just be temporary until 1.0 when hopefully we stop making breaking changes for a bit.

Ok, added it back as a deprecated flag (in Cobra terms this means it'll hide the flag by default and produce a deprecation warning when a value is provided). When provided, we just assign reference to the value instead of explicitly erroring out -- WDYT?

Signed-off-by: Vaughn Dice <vadice@microsoft.com>

carolynvs · 2021-01-12T19:51:45Z

+// A previous iteration of this flag was used to designate an entire bundle
+// reference.  If we detect this attempted use, we return an error and
+// explanation
+func (o *PublishOptions) validateTag() error {


Thanks for adding this! It will really help avoid a bunch of very unhappy users.

Signed-off-by: Vaughn Dice <vadice@microsoft.com>

carolynvs · 2021-01-12T21:23:15Z


+func addDeprecatedTagFlag(f *pflag.FlagSet, opts *porter.BundlePullOptions) {
+	f.StringVar(&opts.Tag, "tag", "", "")
+	f.MarkDeprecated("tag", "use --reference to declare a full bundle reference")


Will this print a warning so that they know it's been deprecated and that they should stop using it? If not, let's an the warning to where you do the reference = tag assignment.

It will. This is what it currently looks like:

$ p install --tag localhost:5000/cert-manager:v0.1.0 -c mk -p cert-manager Flag --tag has been deprecated, use --reference to declare a full bundle reference installing cert-manager... ...

Do we want stronger verbiage (e.g. WARNING ...)?

Nope! I just hadn't used the cobra deprecation flag and wasn't sure how it worked. 👍

carolynvs

One step closer to 1.0!

Signed-off-by: Vaughn Dice <vadice@microsoft.com>

vdice added 3 commits January 7, 2021 09:51

ref(*): rename tag flag/opt to reference where applicable

0604034

Signed-off-by: Vaughn Dice <vadice@microsoft.com>

add tag and registry flags to porter publish

c066d68

Signed-off-by: Vaughn Dice <vadice@microsoft.com>

on publish, tag new inv img instead of triggering rebuild

672f3f4

Signed-off-by: Vaughn Dice <vadice@microsoft.com>

vdice requested review from carolynvs and jeremyrickard as code owners January 8, 2021 17:14

carolynvs self-assigned this Jan 8, 2021

update stamp digest

1cd581d

Signed-off-by: Vaughn Dice <vadice@microsoft.com>

carolynvs reviewed Jan 12, 2021

View reviewed changes

vdice mentioned this pull request Jan 12, 2021

Remove deprecated BundleTag field on the Porter manifest #1408

Closed

vdice added 2 commits January 12, 2021 11:08

add note to long description of publish cmd

f53f40f

Signed-off-by: Vaughn Dice <vadice@microsoft.com>

add tag validation and deprecation error

e3ea05c

Signed-off-by: Vaughn Dice <vadice@microsoft.com>

carolynvs reviewed Jan 12, 2021

View reviewed changes

add back deprecated tag flag to BundlePullOptions

8ec0011

Signed-off-by: Vaughn Dice <vadice@microsoft.com>

carolynvs reviewed Jan 12, 2021

View reviewed changes

carolynvs approved these changes Jan 12, 2021

View reviewed changes

un-parallelize TestConfig_GenerateStamp

12979d2

Signed-off-by: Vaughn Dice <vadice@microsoft.com>

vdice merged commit 0fa366e into getporter:main Jan 12, 2021

vdice deleted the ref/update-publish-flags branch January 12, 2021 22:41

		@@ -47,6 +47,10 @@ type Manifest struct {
		// and isn't meant to be user-specified
		BundleTag string `yaml:"-"`

Conversation

vdice commented Jan 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this change

What issue does it fix

Notes for the reviewer

Checklist

Uh oh!

carolynvs commented Jan 8, 2021

Uh oh!

carolynvs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carolynvs left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vdice commented Jan 8, 2021 •

edited

Loading