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

Fix cli version comparison and improve setup #3518

Merged
merged 6 commits into from
Mar 28, 2024

Conversation

anbraten
Copy link
Member

No description provided.

@anbraten anbraten added bug Something isn't working cli labels Mar 20, 2024
@xoxys
Copy link
Member

xoxys commented Mar 20, 2024

This version check implementation has the same issue as the one in the UI. If a "dev" version from a commit is used, it complains. Is this also fixed? Or expected?

@anbraten
Copy link
Member Author

anbraten commented Mar 20, 2024

For development versions it will skip the check. For pre-releases / next versions it will show you the hint to update to the latest stable, that should be okay however, I guess.

@xoxys
Copy link
Member

xoxys commented Mar 20, 2024

Thanks 👍

@qwerty287
Copy link
Contributor

@anbraten two questions about the version:

  1. What happens with next- version?
  2. Why don't we reuse https://woodpecker-ci.org/version.json? This would allow to also suggest RCs if you're currently on a RC

@xoxys
Copy link
Member

xoxys commented Mar 20, 2024

Should answer "What happens with next- version?"

For pre-releases next versions it will show you the hint to update to the latest stable

@qwerty287
Copy link
Contributor

Ah sorry, I missed the word "next" there and thought you mean RCs...
Still I think it would be better to properly support next and show next updates - shouldn't be that hard using version.json.

@qwerty287 qwerty287 added this to the 2.5.0 milestone Mar 21, 2024
@anbraten
Copy link
Member Author

Adjusted the code to use our version.json file. We could potentially run into a 404 error as we don't know if the binary is actually uploaded (yet) if we see a new version, but I think that shouldn't be an issue and the user will simply get an error in that case.

@woodpecker-bot
Copy link
Collaborator

woodpecker-bot commented Mar 27, 2024

Deployment of preview was successful: https://woodpecker-ci-woodpecker-pr-3518.surge.sh

Copy link
Contributor

@qwerty287 qwerty287 left a comment

Choose a reason for hiding this comment

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

code looks good, untested

Copy link

codecov bot commented Mar 28, 2024

Codecov Report

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

Project coverage is 35.82%. Comparing base (326069c) to head (4171976).
Report is 13 commits behind head on main.

Files Patch % Lines
cli/update/updater.go 47.61% 6 Missing and 5 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3518      +/-   ##
==========================================
- Coverage   36.04%   35.82%   -0.23%     
==========================================
  Files         229      232       +3     
  Lines       15493    15668     +175     
==========================================
+ Hits         5585     5613      +28     
- Misses       9502     9636     +134     
- Partials      406      419      +13     

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

@anbraten
Copy link
Member Author

anbraten commented Mar 28, 2024

Fixed the wrong url string in the code and added a test. Also tested it again. Seems to work now.

@anbraten anbraten merged commit c2a8464 into woodpecker-ci:main Mar 28, 2024
8 of 9 checks passed
@anbraten anbraten deleted the fix-cli-setup branch March 28, 2024 09:36
@@ -58,7 +58,7 @@ func After(_ *cli.Context) error {
select {
case <-waitForUpdateCheck.Done():
// When the actual command already finished, we still wait 250ms for the update check to finish
Copy link
Member

Choose a reason for hiding this comment

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

Desc is wrong now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cli
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants