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

Fail kudo init if version is unknown. #1556

Merged
merged 6 commits into from Jun 24, 2020
Merged

Fail kudo init if version is unknown. #1556

merged 6 commits into from Jun 24, 2020

Conversation

porridge
Copy link
Member

@porridge porridge commented Jun 9, 2020

What this PR does / why we need it:

This hopefully should have no impact on developer workflows nor on how a (pre-)released build works. However in cases where a matching controller docker image does not exist, it will fail fast, rather than deploying a (potentially incompatible) controller from a docker image built for some earlier commit.

Implementation:

  • first, set pkg/version.gitVersion to a version string using $LDFLAGS only when a tagged commit is checked out (this will always be true for release builds). In other cases, set it to a sentinel value.
  • then refuse to kudo init unless the sentinel value is overridden explicitly.

Fixes #1549

Signed-off-by: Marcin Owsiany mowsiany@D2iQ.com

Signed-off-by: Marcin Owsiany <mowsiany@D2iQ.com>
Signed-off-by: Marcin Owsiany <mowsiany@D2iQ.com>
Signed-off-by: Marcin Owsiany <mowsiany@D2iQ.com>
@porridge porridge marked this pull request as ready for review June 9, 2020 09:08
Copy link
Member

@kensipe kensipe left a comment

Choose a reason for hiding this comment

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

I love this! 2 Comments, 1 blocking...

  1. non-blocking: not-built-on-tag is technically correct but I'm not sure conveys full meaning. not-built-on-release ? I'm ok with what is there... just a thought.
  2. --image is not the only way to over the image... (which this change assumes). --version also works and is likely the preferred method to get the image from the official source. --version is to switch a version. --image is for switch docker repository.

@@ -142,6 +142,8 @@ func (initCmd *initCmd) run() error {
// if image provided switch to it.
if initCmd.image != "" {
opts.Image = initCmd.image
} else if opts.Version == "not-built-on-tag" {
return errors.New("cannot infer controller docker image to use, please override with --kudo-image")
Copy link
Member

Choose a reason for hiding this comment

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

👏 I like it!

Signed-off-by: Marcin Owsiany <mowsiany@D2iQ.com>
@porridge
Copy link
Member Author

porridge commented Jun 22, 2020

PTAL, I:

  • changed the sentinel value as you suggested. I think it conveys the intent beter.
  • removed the exact flag name from the error message, and added a mention of "not released", since we probably want to use this as a hint that "maybe you should not be building this yourself" rather than triggering a knee-jerk reaction of using --version. If someone really wants to use it this way, they should be able to find the right flag from the kubectl kudo help init.
  • redirected errors from the tag discovery command to /dev/null

@kensipe kensipe changed the base branch from master to main June 24, 2020 00:37
…ild not built from tag

Signed-off-by: Ken Sipe <kensipe@gmail.com>
Copy link
Member

@kensipe kensipe left a comment

Choose a reason for hiding this comment

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

love this!
/lgtm

updated code to have the dev experience to be consistent with non-release build

go run cmd/kubectl-kudo/main.go init
Error: cannot infer controller docker image to use - not a released binary; please override with a command-line flag
exit status 255

Signed-off-by: Ken Sipe <kensipe@gmail.com>
@kensipe kensipe merged commit 80f5a21 into main Jun 24, 2020
@kensipe kensipe deleted the porridge/fail-init branch June 24, 2020 14:18
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.

Highly sub-optimal UX of make cli-install at HEAD of master
2 participants