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

Replacing complicated latest version commands with simple curl #919

Merged
merged 1 commit into from
Nov 15, 2023

Conversation

cwilkers
Copy link
Member

@cwilkers cwilkers commented Oct 9, 2023

What this PR does / why we need it:

Replace complicated multi-command pipeline to get latest stable version string with an easy curl in our quickstart documentation.

Does this PR fix any issue? (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):

Fixes #891

Special notes for your reviewer:

Signed-off-by: cwilkers <cwilkers@redhat.com>
@kubevirt-bot kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/XS labels Oct 9, 2023
@cwilkers
Copy link
Member Author

Hi @fabiand @dhiller @dankenigsberg @0xFelix @aburdenthehand ,

I just added you as reviewers because I think this is a deceptively important Intro to KubeVirt kind of issue.

Our tooling seems to support a very concise and easy way to determine the latest version of KubeVirt that solves a number of problems, some of which have been raised in recent issues.

I'm hoping to answer one big questions by inviting so many busy (but highly involved) people as reviewer:

  1. Is this a sustainable way to get the stable version? i.e. are we taking advantage of a side-effect that may go away in the future due to changes in the CI processes?

Advantages:

  1. I have seen recent blog posts using this command instead of the convoluted one in our intros, and thought, "Why aren't we presenting this in our labs?"
  2. Older ways involved multi-pipe bash commands in a $() setup that would make a greybeard Un*x admin shed a tear of pride, but may make newer users flee in horror
  3. The new method here avoids alpha and -rcX releases so our demo and 101 content will work on latest stable versions only. Deployment code block doesn't filter alpha releases #891
  4. Our minikube plugin is currently broken (in a way that sadly this does not fix) but simplifying the curl statement will certainly help. See KubeVirt Addon broken due to missing curl in Pod kubernetes/minikube#15918 for why minikube is broken.

Thank you all for your time!

@dankenigsberg
Copy link
Member

Few notes.

  • I don't really mind having a multipipe to extract the version. Note that my beard is grey.
  • I don't know what generates https://storage.googleapis.com/kubevirt-prow/release/kubevirt/kubevirt/stable.txt and if it can be trusted to stay up-to-date forever. @brianmcarey may know.
  • I find it less nice to depend on an external service; but we're "married" so strongly to Prow anyway that adding this is does not matter.
  • The multipipe is buggy. sort -r says that 1.9 is newer than 1.10. If you keep the multipipe, make it sort -rV.

@0xFelix
Copy link
Member

0xFelix commented Nov 15, 2023

I agree, we should make this commandline a bit friendlier. :)

And since we are currently using the GitHub API, may I throw in the following?

curl -s https://api.github.com/repos/kubevirt/kubevirt/releases/latest | jq -r '.tag_name'

If we don't want to rely on Prow this looks to be the easiest solution to me.

@brianmcarey
Copy link
Member

* I don't know what generates https://storage.googleapis.com/kubevirt-prow/release/kubevirt/kubevirt/stable.txt and if it can be trusted to stay up-to-date forever. @brianmcarey may know.

That stable.txt file is updated as part of the release process so I think it is ok to rely on this. The release will not be created successfully if this file is not updated.

https://github.com/kubevirt/kubevirt/blob/d41b6ddb20837e4445a899be0dc492bde25beb26/automation/release.sh#L91

@aburdenthehand
Copy link
Contributor

/lgtm
@0xFelix That is a nice and concise command you've got there :) I like it. One nit (from my writer days) is that jq isn't included by default and could be an additional install for some folks.

From Brian's comment this source seems sufficiently reliable, and the simplicity is attractive.

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 15, 2023
@dhiller
Copy link
Contributor

dhiller commented Nov 15, 2023

What @brianmcarey said.

Also we are advertising this URL in the installation section of our user-guide: https://kubevirt.io/user-guide/operations/installation/#installing-kubevirt-on-kubernetes

So it will not go away and should be as reliable as google cloud storage is.... Besides that, if GCS is not working, prow will not be working.

@0xFelix
Copy link
Member

0xFelix commented Nov 15, 2023

I'd vote to keep the GCS curl. Looks to be the most compatible, easy to understand and robust solution.

/lgtm

@cwilkers
Copy link
Member Author

I think there's a consensus here, I'll go ahead and ...
/approve

@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cwilkers

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 15, 2023
@kubevirt-bot kubevirt-bot merged commit e17198c into kubevirt:main Nov 15, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. size/XS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deployment code block doesn't filter alpha releases
7 participants