Include tags when --push=false is set #689
Conversation
|
This Pull Request is stale because it has been open for 90 days with |
There was a problem hiding this comment.
Hey Adam, sorry I let this PR linger for so long! The change looks good, if you're still interested in pursuing it.
I think it would be helpful to have some test that checks that the original buggy behavior is fixed by this change, otherwise it's a bit hard to tell how the noop publisher refactor is the solution. Future changes to noop publishing might regress back to the buggy behavior without a backstop unit test.
If that's something you're interested in, I promise to review it more quickly this time 😞
pkg/publish/default.go
Outdated
| // Option is a functional option for NewDefault. | ||
| type Option func(*defaultOpener) error | ||
| // DefaultOption is a functional option for NewDefault. | ||
| type DefaultOption func(*defaultOpener) error |
There was a problem hiding this comment.
This change will probably cause some pain to consumers of this package as a library. Can we keep this as Option?
- Refactor noop into its own publisher. - Use :tag@sha format in digest when --push=false is set. - Allow --tag-only to be used with --push=false. This includes validations that prevent --tag-only to be used with no tags or the `latest` tag. Signed-off-by: Adam Kaplan <[email protected]>
adambkaplan
force-pushed
the
tags-with-no-push
branch
from
b2bd320
to
f508b06
Compare
|
@imjasonh finally got around to adding an e2e test. Hopefully we can get this in before the big move to ko-build! |
| repoName: repoName, | ||
| namer: namer, | ||
| }) | ||
| noop, err := publish.NewNoOp(repoName, |
There was a problem hiding this comment.
Unless the intention here is to export more functionality around no-op publishing, I'd kind of like to avoid increasing the public surface area of this change. We can do that just by passing more fields to nopPublisher and not exporting anything new.
If the intention is to export more no-op publishing functionality, I'd like to understand more about the use case so we can make sure this change helps the issue.
In any case I think we can get this in before v0.12 and the repo move, if you're interested.
There was a problem hiding this comment.
I don't think there is any further no-op publishing functionality to export.
There was a problem hiding this comment.
What I'm saying is, this change exports new methods in pkg/publish to enable no-op publishing. But that's not necessary just to get the CLI behavior you're proposing -- we could do that just by passing more options into the (unexported) nopPublisher struct.
I think we should try to get this CLI behavior change done without exporting new methods, since once we export it, we're married to it forever. AFAIK there aren't a lot of use cases for no-op publishing, vs local publishing or tarball publishing, which are better supported don't-push-to-a-registry options, except in this very narrow use case.
validations that prevent --tag-only to be used with no tags or the
latesttag.Fixes #688