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

cli: add 'tilt dump image-deploy-ref', for determining the deploy tag of an image #3607

Merged
merged 1 commit into from
Jul 23, 2020

Conversation

nicks
Copy link
Member

@nicks nicks commented Jul 22, 2020

Hello @maiamcc, @landism,

Please review the following commits I made in branch nicks/ch8524:

d77706b (2020-07-22 16:58:12 -0400)
cli: add 'tilt dump image-deploy-ref', for determining the deploy tag of an image
helps with more complex custom_build commands as described in #3584

Code review reminders, by giving a LGTM you attest that:

  • Commits are adequately tested
  • Code is easy to understand and conforms to style guides
  • Incomplete code is marked with TODOs
  • Code is suitably instrumented with logging and metrics

Copy link
Contributor

@maiamcc maiamcc left a comment

Choose a reason for hiding this comment

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

nice!

func newDumpImageDeployRefCmd() *cobra.Command {
return &cobra.Command{
Use: "image-deploy-ref REF",
Short: "Determine the deployed name and tag of a custom-built image",
Copy link
Contributor

Choose a reason for hiding this comment

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

idk if this is actually clearer, take it or leave it 🤷

Suggested change
Short: "Determine the deployed name and tag of a custom-built image",
Short: "Determine the name and tag with which Tilt will deploy the given custom-built image",

(and change in Long description as well)

Copy link
Member Author

Choose a reason for hiding this comment

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

done!

return nil, errors.Wrap(err, "DumpImageDeployRef")
}

data, _, err := d.dCli.ImageInspectWithRaw(ctx, ref)
Copy link
Contributor

Choose a reason for hiding this comment

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

Have a vague instinct that as much of this logic as possible should live in a helper function that's called by both dump-image... and the CustomBuilder, to guard against the logic going out of sync. How hard would that be?

Copy link
Member Author

Choose a reason for hiding this comment

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

ya, that's how it works!

the helper function is digestAsTag.

everything else is type conversion, for types that are different in the two codepaths for good reasons.

Copy link
Contributor

Choose a reason for hiding this comment

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

whoops yes. cool!

@maiamcc
Copy link
Contributor

maiamcc commented Jul 23, 2020

Also are you going to update the Custom Build docs to reflect this strategy?

@nicks nicks force-pushed the nicks/ch8524 branch 2 times, most recently from db5e716 to 00eae28 Compare July 23, 2020 19:12
@nicks
Copy link
Member Author

nicks commented Jul 23, 2020

yep, the attached issue outlines what the new docs will look like. it's a bit of an advanced feature (and i'm concerned that more people will use it incorrectly than use it correctly). But we're pairing with a partner team and watching closely how they use it

… of an image

helps with more complex custom_build commands as described in #3584
@nicks nicks merged commit 426c6e0 into master Jul 23, 2020
@nicks nicks deleted the nicks/ch8524 branch July 23, 2020 19:50
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.

None yet

2 participants