-
Notifications
You must be signed in to change notification settings - Fork 389
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change will probably cause some pain to consumers of this package as a library. Can we keep this as Option
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted.
- 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 <adam@adambkaplan.com>
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there is any further no-op publishing functionality to export.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
latest
tag.Fixes #688