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

Warn users if not using latest release. #1570

Merged
merged 3 commits into from
Apr 19, 2024

Conversation

AlexVulaj
Copy link
Contributor

@AlexVulaj AlexVulaj commented Oct 31, 2023

OCM-4624 | feat: Show a warning when a user attempts to run a command on a version other than the latest version from mirror.

Sample run from a local build:

$ rosa create cluster
E: The current version (1.2.27) is not up to date with latest released version (1.2.28).
E: It is recommended that you update to the latest version.
...

cmd/rosa/main.go Outdated Show resolved Hide resolved
cmd/rosa/main.go Outdated Show resolved Hide resolved
cmd/rosa/main.go Outdated Show resolved Hide resolved
@AlexVulaj AlexVulaj force-pushed the version-prompt branch 4 times, most recently from fbcae0d to 3b027d8 Compare November 1, 2023 13:30
@ciaranRoche
Copy link
Member

/retest-required

@AlexVulaj
Copy link
Contributor Author

/hold

Waiting for input from UX

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 1, 2023
@AlexVulaj
Copy link
Contributor Author

/retest-required

cmd/rosa/main.go Outdated Show resolved Hide resolved
cmd/rosa/main.go Outdated Show resolved Hide resolved
@AlexVulaj AlexVulaj force-pushed the version-prompt branch 3 times, most recently from 5a74ea5 to 98cabf9 Compare November 8, 2023 16:01
@AlexVulaj AlexVulaj changed the title Prompt users to continue if not using latest release. Warn users if not using latest release. Nov 8, 2023
@andreadecorte
Copy link
Contributor

should we have some local caching first? This is doing a call for version check at every single rosa command which may add a significant traffic on the mirror, but versions don't change every second

@thomasmckay
Copy link

should we have some local caching first? This is doing a call for version check at every single rosa command which may add a significant traffic on the mirror, but versions don't change every second

After team discussion, we'd prefer to not cache until it's a known problem (we believe that is unlikely). In addition, it would be interesting/beneficial to track which commands are being run.

@AlexVulaj
Copy link
Contributor Author

@ciaranRoche There's still a prow build failing that looks strange to me, any thoughts on what might be going on?

@ciaranRoche
Copy link
Member

/test images-images

@cben
Copy link
Contributor

cben commented Dec 5, 2023

Will rosa version and rosa verify rosa-client commands now make the check (and possibly print the message) twice?

I guess at least this line making rosa version also run the verify rosa-client logic can be dropped:

rosa.Cmd.Run(rosa.Cmd, []string{})

cmd/rosa/main.go Outdated Show resolved Hide resolved
@cben
Copy link
Contributor

cben commented Dec 5, 2023

After team discussion, we'd prefer to not cache until it's a known problem (we believe that is unlikely).

For me, rosa verify rosa-client alone takes ~0.9sec.
(ping mirror.openshift.com is 65–75ms RTT for me, so does it take nearly a second for the HTTP server to send the directory listing?)

So with this change, every command would become slightly, but perceptibly slower.
I'd not love that even for interactive use.
It can get worse, both for speed and traffic, when scripted in a loop... 🔃

  • Worth confirming with mirror.openshift.com admins they don't mind getting all that traffic?

  • This header is interesting (using version, verify rosa-client: Support --debug #1640, 100% reproducible)

    time=2023-12-05T20:30:43+02:00 level=debug msg=Response header 'X-Cache' is 'Miss from cloudfront'
    

    In theory, a directory listing could be much faster if cached at CDN level. Is it un-cachable deliberately (invalidation is hard) or just oversight?

@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 6, 2024
Copy link

codecov bot commented Feb 6, 2024

Codecov Report

Attention: Patch coverage is 17.39130% with 19 lines in your changes are missing coverage. Please review.

Project coverage is 22.44%. Comparing base (b480a5d) to head (d7bf0f4).
Report is 16 commits behind head on master.

Files Patch % Lines
cmd/rosa/main.go 0.00% 19 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1570      +/-   ##
==========================================
+ Coverage   21.71%   22.44%   +0.72%     
==========================================
  Files         118      129      +11     
  Lines       19809    20574     +765     
==========================================
+ Hits         4302     4617     +315     
- Misses      15186    15609     +423     
- Partials      321      348      +27     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@robpblake
Copy link
Contributor

See #1777

cmd/rosa/main.go Outdated Show resolved Hide resolved
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 17, 2024
cmd/rosa/main.go Outdated Show resolved Hide resolved
… on a version other than the latest version from mirror.
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 17, 2024
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 17, 2024
@AlexVulaj AlexVulaj force-pushed the version-prompt branch 2 times, most recently from c567e54 to fdcfcba Compare April 17, 2024 17:53
@AlexVulaj
Copy link
Contributor Author

/retest

… on a version other than the latest version from mirror.
Copy link
Member

@ciaranRoche ciaranRoche left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks for you patience on this one Alex, glad to have this check in place when it lands.

@robpblake can you give this a final ack before we merge 🙏

@robpblake
Copy link
Contributor

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 19, 2024
Copy link
Contributor

@robpblake robpblake left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 19, 2024
Copy link
Contributor

openshift-ci bot commented Apr 19, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AlexVulaj, ciaranRoche, robpblake

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:
  • OWNERS [ciaranRoche,robpblake]

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

Copy link
Contributor

openshift-ci bot commented Apr 19, 2024

@AlexVulaj: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@openshift-merge-bot openshift-merge-bot bot merged commit 71d3a04 into openshift:master Apr 19, 2024
9 checks passed
@AlexVulaj AlexVulaj deleted the version-prompt branch April 19, 2024 15:35
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. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants