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

mimir-build-image: Upgrade Go version to 1.23.0-bookworm #9070

Merged
merged 16 commits into from
Aug 23, 2024

Conversation

alexweav
Copy link
Contributor

@alexweav alexweav commented Aug 21, 2024

What this PR does

Upgrades Go version in mimir-build-image.

This required an upgrade to golangci-lint. In turn, the upgrade of this introduced a new linter rule for constant format strings (which in rare cases can be a security thing). Fixed all violations of this new linter rule.

Which issue(s) this PR fixes or relates to

n/a

Checklist

  • Tests updated.
  • Documentation added.
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • about-versioning.md updated with experimental features.

@alexweav alexweav requested a review from a team as a code owner August 21, 2024 20:44
Copy link
Contributor

Building new version of mimir-build-image. After image is built and pushed to Docker Hub, a new commit will automatically be added to this PR with new image version grafana/mimir-build-image:pr9070-2850516d73. This can take up to 1 hour.

@alexweav
Copy link
Contributor Author

Not ready to merge yet: Awaiting image to be built and bot commit per instructions https://github.com/grafana/mimir/blob/main/docs/internal/how-to-update-the-build-image.md?plain=1

Copy link
Contributor

Not building new version of mimir-build-image. This PR modifies mimir-build-image/Dockerfile, but the image grafana/mimir-build-image:pr9070-2850516d73 already exists.

Copy link
Contributor

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

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

I think you have to upgrade to the latest version of golangci-lint also. That's probably why the lint CI job failed.

Copy link
Contributor

Building new version of mimir-build-image. After image is built and pushed to Docker Hub, a new commit will automatically be added to this PR with new image version grafana/mimir-build-image:pr9070-3b4c4993ed. This can take up to 1 hour.

Copy link
Contributor

Not building new version of mimir-build-image. This PR modifies mimir-build-image/Dockerfile, but the image grafana/mimir-build-image:pr9070-3b4c4993ed already exists.

@alexweav
Copy link
Contributor Author

Thanks @aknuds1 😄

The remaining lint errors seem to be new/legitimate ones caught by the upgraded linter - i'll post another commit fixing these up tomorrow.

@alexweav alexweav force-pushed the alexweav/upgrade-build-image-go-1-23-0 branch from e9ef469 to cc76be0 Compare August 23, 2024 15:15
Copy link
Contributor

Building new version of mimir-build-image. After image is built and pushed to Docker Hub, a new commit will automatically be added to this PR with new image version grafana/mimir-build-image:pr9070-5347ec889b. This can take up to 1 hour.

Copy link
Contributor

Building new version of mimir-build-image. After image is built and pushed to Docker Hub, a new commit will automatically be added to this PR with new image version grafana/mimir-build-image:pr9070-5347ec889b. This can take up to 1 hour.

Copy link
Contributor

Building new version of mimir-build-image. After image is built and pushed to Docker Hub, a new commit will automatically be added to this PR with new image version grafana/mimir-build-image:pr9070-5347ec889b. This can take up to 1 hour.

Copy link
Contributor

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

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

Please see suggestions for cleanup.

pkg/distributor/distributor_ingest_storage_test.go Outdated Show resolved Hide resolved
pkg/distributor/otel.go Outdated Show resolved Hide resolved
pkg/distributor/otel.go Outdated Show resolved Hide resolved
pkg/distributor/otel.go Outdated Show resolved Hide resolved
pkg/distributor/push.go Outdated Show resolved Hide resolved
pkg/distributor/push_test.go Outdated Show resolved Hide resolved
pkg/scheduler/scheduler.go Outdated Show resolved Hide resolved
tools/tsdb-index-toc/main.go Outdated Show resolved Hide resolved
tools/tsdb-index-toc/main.go Outdated Show resolved Hide resolved
tools/tsdb-index-toc/main.go Outdated Show resolved Hide resolved
Copy link
Contributor

Building new version of mimir-build-image. After image is built and pushed to Docker Hub, a new commit will automatically be added to this PR with new image version grafana/mimir-build-image:pr9070-5347ec889b. This can take up to 1 hour.

Copy link
Contributor

Building new version of mimir-build-image. After image is built and pushed to Docker Hub, a new commit will automatically be added to this PR with new image version grafana/mimir-build-image:pr9070-5347ec889b. This can take up to 1 hour.

Copy link
Contributor

Building new version of mimir-build-image. After image is built and pushed to Docker Hub, a new commit will automatically be added to this PR with new image version grafana/mimir-build-image:pr9070-5347ec889b. This can take up to 1 hour.

Copy link
Contributor

Building new version of mimir-build-image. After image is built and pushed to Docker Hub, a new commit will automatically be added to this PR with new image version grafana/mimir-build-image:pr9070-5347ec889b. This can take up to 1 hour.

Copy link
Contributor

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

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

Left a nit, I think linting will still fail for a couple of cases.

Copy link
Contributor

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

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

Sorry, my suggestion got lost in the previous review.

pkg/scheduler/scheduler.go Outdated Show resolved Hide resolved
@alexweav
Copy link
Contributor Author

Indeed, I do wish the linter would report all the errors at once on the CI rather than reporting them in batches.

@aknuds1
Copy link
Contributor

aknuds1 commented Aug 23, 2024

Indeed, I do wish the linter would report all the errors at once on the CI rather than reporting them in batches.

It's the easiest to lint locally.

Copy link
Contributor

Building new version of mimir-build-image. After image is built and pushed to Docker Hub, a new commit will automatically be added to this PR with new image version grafana/mimir-build-image:pr9070-5347ec889b. This can take up to 1 hour.

@alexweav
Copy link
Contributor Author

Yeah, having to fall back to that it seems. I was misled by the last round (having only one error reported) into thinking I am done 🙈

Copy link
Contributor

Building new version of mimir-build-image. After image is built and pushed to Docker Hub, a new commit will automatically be added to this PR with new image version grafana/mimir-build-image:pr9070-5347ec889b. This can take up to 1 hour.

Copy link
Contributor

@aknuds1 aknuds1 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!

@alexweav
Copy link
Contributor Author

Thanks for the review @aknuds1!

@aknuds1 aknuds1 force-pushed the alexweav/upgrade-build-image-go-1-23-0 branch from 86c300f to 4bed73e Compare August 23, 2024 17:00
Copy link
Contributor

Not building new version of mimir-build-image. This PR modifies mimir-build-image/Dockerfile, but the image grafana/mimir-build-image:pr9070-5347ec889b already exists.

@aknuds1 aknuds1 enabled auto-merge (squash) August 23, 2024 17:01
@aknuds1 aknuds1 merged commit 87f601f into main Aug 23, 2024
30 checks passed
@aknuds1 aknuds1 deleted the alexweav/upgrade-build-image-go-1-23-0 branch August 23, 2024 17:17
@narqo narqo mentioned this pull request Sep 3, 2024
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