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

refactor: order by key when printing labels #960

Merged
merged 1 commit into from
Mar 7, 2025
Merged

refactor: order by key when printing labels #960

merged 1 commit into from
Mar 7, 2025

Conversation

phm07
Copy link
Contributor

@phm07 phm07 commented Jan 17, 2025

This PR adds sorting of labels by their keys, which is useful for testing, since iteration order over maps in Go is undefined by default.
It uses the Go 1.23 iteration feature. If we want to use this, we should probably bump the Go version in a separate PR.

@phm07 phm07 self-assigned this Jan 17, 2025
@phm07 phm07 requested a review from a team as a code owner January 17, 2025 16:15
Copy link

codecov bot commented Jan 17, 2025

Codecov Report

Attention: Patch coverage is 63.15789% with 7 lines in your changes missing coverage. Please review.

Project coverage is 70.17%. Comparing base (138a10a) to head (3514764).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
internal/cmd/util/util.go 62.50% 2 Missing and 1 partial ⚠️
internal/cmd/loadbalancer/describe.go 0.00% 1 Missing ⚠️
internal/cmd/primaryip/describe.go 0.00% 1 Missing ⚠️
internal/cmd/server/describe.go 0.00% 1 Missing ⚠️
internal/cmd/volume/describe.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #960      +/-   ##
==========================================
+ Coverage   63.66%   70.17%   +6.50%     
==========================================
  Files         243      245       +2     
  Lines       10719    10782      +63     
==========================================
+ Hits         6824     7566     +742     
+ Misses       3180     2539     -641     
+ Partials      715      677      -38     
Flag Coverage Δ
e2e 46.94% <57.89%> (?)
unit 63.63% <57.89%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@apricote
Copy link
Member

Personally I do not think this is worth bumping the Go dependency before 1.24 is released and 1.22 becomes EOL anyway.

@phm07
Copy link
Contributor Author

phm07 commented Jan 22, 2025

Personally I do not think this is worth bumping the Go dependency before 1.24 is released and 1.22 becomes EOL anyway.

I initially created this PR because the undeterministic behavior of label output made it difficult to test output in the e2e tests. Since the CLI is not a library I don't see any drawbacks from upgrading to a newer Go version.

On that note, we could also test the new tool directive here once Go 1.24 releases.

@phm07
Copy link
Contributor Author

phm07 commented Jan 22, 2025

This already causes the CI to fail randomly: https://github.com/hetznercloud/cli/actions/runs/12893320728/job/35949486386#step:5:527

@phm07
Copy link
Contributor Author

phm07 commented Mar 7, 2025

Since #991 was merged, we now require Go 1.23, so this PR doesn't bump the Go version anymore.

@apricote apricote changed the title feat: order by key when printing labels refactor: order by key when printing labels Mar 7, 2025
@apricote
Copy link
Member

apricote commented Mar 7, 2025

I do not think this is worthy of being added to the Changelog, does not really matter for customers and is more about test behaviour. I updated the title for this.

@phm07 phm07 merged commit 944953f into main Mar 7, 2025
8 checks passed
@phm07 phm07 deleted the order-map-keys branch March 7, 2025 13:12
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.

2 participants