Skip to content

Embed Tagger in Builder #802

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

Closed
wants to merge 1 commit into from

Conversation

nkubala
Copy link
Contributor

@nkubala nkubala commented Jul 9, 2018

Tags are never resolved without build context, so rather than passing around the Runner's Tagger to Build, we can just embed the Tagger into the Builder itself.

@r2d4
Copy link
Contributor

r2d4 commented Jul 9, 2018

Thats true, but in the future it might be useful to generate the tag without the build context. What's the motivation here?

@r2d4
Copy link
Contributor

r2d4 commented Jul 9, 2018

A bit hand-wavy, but we've talked about separating "stable taggers" from ones that depend on the build context, like the checksum tagger. That would allow us to generate the tag for commands like skaffold deploy without the user explicitly specifying it.

@nkubala
Copy link
Contributor Author

nkubala commented Jul 9, 2018

A bit hand-wavy, but we've talked about separating "stable taggers" from ones that depend on the build context, like the checksum tagger.

Hmm interesting. Those approaches aren't mutually exclusive, we could always have embedded taggers in each builder created by the runner but also expose stable taggers to be used outside of the normal skaffold dev loop.

The main motivation is to make it easier to retrieve information about a skaffold run without actually doing the build/deploy (see #726). This is more of a "dry run" approach but it would be useful to see what skaffold would have done without actually doing it. I realize this won't be possible with all builder/tagger combinations (e.g. GoogleCloudBuilder uses the digest retrieved from the completed build as part of the tag), but in general it makes more sense to abstract constructing the &tag.Options inside the builder IMO.

@dlorenc
Copy link
Contributor

dlorenc commented Jul 16, 2018

Is this PR still active?

@balopat
Copy link
Contributor

balopat commented Aug 3, 2018

Another hand-wavey issue I see with this approach is that it doesn't express well that "a builder should build an array of artifacts that are tagged using a specified tagger" because now it becomes an implementation detail to the builder that it might or might not use taggers. The fact that we inject the tagger in the method is more expressive - even though you can still implement a builder without tagger.

@nkubala why is it "... easier to retrieve information about a skaffold run without actually doing the build/deploy" this way?

@balopat
Copy link
Contributor

balopat commented Aug 6, 2018

Let's close this - #653 is another pointer why it would be better to keep tagging separate. @nkubala wdyt?

@nkubala nkubala closed this Aug 8, 2018
@nkubala nkubala deleted the embed_tag branch June 12, 2019 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants