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

chore: bump go version to 1.22 #1112

Merged
merged 6 commits into from
Oct 15, 2024
Merged

chore: bump go version to 1.22 #1112

merged 6 commits into from
Oct 15, 2024

Conversation

s4kh
Copy link
Contributor

@s4kh s4kh commented Oct 14, 2024

What this PR does / why we need it:
To allow otel dependency updates (which dropped support for Go 1.21)

Which issue(s) this PR fixes:

Fixes #1107
Fixes #1079

Special notes for your reviewer:
Updated grafana/grafana-plugin-examples#396 grafana/plugin-tools#1212

@s4kh s4kh requested a review from a team as a code owner October 14, 2024 16:33
@s4kh s4kh requested review from wbrowne, andresmgot and xnyo and removed request for a team October 14, 2024 16:33
@CLAassistant
Copy link

CLAassistant commented Oct 14, 2024

CLA assistant check
All committers have signed the CLA.

@s4kh s4kh marked this pull request as draft October 14, 2024 16:34
@s4kh s4kh force-pushed the chore-bump-go-version branch from 82cb31b to d74ec1f Compare October 14, 2024 16:36
Copy link
Contributor

@marefr marefr left a comment

Choose a reason for hiding this comment

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

Wonder what go version is used by grafana-plugin-ci in drone.yml (build) and whether we need to update that.

Always confused by where grafana/grafana-plugin-ci docker image are defined/pushed from. This is one of the benefits migrating from drone to github actions (that other issue) so maybe we can wait with updating drone and migrate to github actions after this is merged. WDYT?

go.mod Outdated Show resolved Hide resolved
@s4kh s4kh force-pushed the chore-bump-go-version branch 3 times, most recently from c7851c7 to 7d1f883 Compare October 14, 2024 20:19
@s4kh
Copy link
Contributor Author

s4kh commented Oct 14, 2024

Wonder what go version is used by grafana-plugin-ci in drone.yml (build) and whether we need to update that.

Always confused by where grafana/grafana-plugin-ci docker image are defined/pushed from. This is one of the benefits migrating from drone to github actions (that other issue) so maybe we can wait with updating drone and migrate to github actions after this is merged. WDYT?

https://github.com/grafana/grafana-drone-extension/blob/main/images/grafana-plugin-ci/scripts/deploy.sh#L22 so the go version here is 1.21.3.

For migrating to GA using grafana/grafana-plugin-ci:1.9.5 is not required, right? Currently, I am iterating on this.

@s4kh s4kh force-pushed the chore-bump-go-version branch 2 times, most recently from ee958ec to 25c8440 Compare October 15, 2024 03:31
Copy link
Contributor

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

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

The approach sounds good to me, not sure why you are getting compilation errors

Copy link
Contributor

@marefr marefr 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 can lint, build and test in the same job perhaps to simplify? Have you run go mod tidy after update of go.mod?

@s4kh s4kh force-pushed the chore-bump-go-version branch from d59787f to ef8c5f7 Compare October 15, 2024 12:10
@s4kh s4kh force-pushed the chore-bump-go-version branch from ef8c5f7 to c1136c8 Compare October 15, 2024 12:20
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@s4kh s4kh marked this pull request as ready for review October 15, 2024 12:29
@s4kh
Copy link
Contributor Author

s4kh commented Oct 15, 2024

@andresmgot @marefr
Apparently, when you wake up and try again, it works. Classic coding 🏛️

@s4kh
Copy link
Contributor Author

s4kh commented Oct 15, 2024

implements #1079

@marefr
Copy link
Contributor

marefr commented Oct 15, 2024

implements #1079

Please add this to the PR description as Fixes #1079 so that it auto-close after this is merged

.remove-drone.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@s4kh s4kh requested review from a team as code owners October 15, 2024 14:20
@s4kh
Copy link
Contributor Author

s4kh commented Oct 15, 2024

@marefr, @xnyo Is there a webhook I need to disable continuous-integration/drone/pr?

Copy link
Contributor

@marefr marefr left a comment

Choose a reason for hiding this comment

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

Great work. Some minor things left

backend/json.go Outdated Show resolved Hide resolved
experimental/concurrent/concurrent.go Outdated Show resolved Hide resolved
experimental/golden_response_checker.go Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@s4kh s4kh force-pushed the chore-bump-go-version branch 5 times, most recently from e2db36b to 3c5c420 Compare October 15, 2024 15:27
@s4kh s4kh requested a review from marefr October 15, 2024 15:29
@xnyo
Copy link
Member

xnyo commented Oct 15, 2024

@marefr, @xnyo Is there a webhook I need to disable continuous-integration/drone/pr?

@s4kh I think we just have to disable the repository in Drone: https://drone.grafana.net/grafana/grafana-plugin-sdk-go/settings

We have to do that after we merge this PR tho

Copy link
Member

@xnyo xnyo left a comment

Choose a reason for hiding this comment

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

Awesome work! 🙌 👏 👏

Just a small nit, but LGTM otherwise

.github/workflows/go-lint.yml Outdated Show resolved Hide resolved
@s4kh s4kh force-pushed the chore-bump-go-version branch 3 times, most recently from 5f4657d to 355e782 Compare October 15, 2024 15:52
Copy link
Contributor

@marefr marefr left a comment

Choose a reason for hiding this comment

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

LGTM with a suggestion

.github/workflows/go-lint.yml Outdated Show resolved Hide resolved
@marefr
Copy link
Contributor

marefr commented Oct 15, 2024

@marefr, @xnyo Is there a webhook I need to disable continuous-integration/drone/pr?

@s4kh I think we just have to disable the repository in Drone: https://drone.grafana.net/grafana/grafana-plugin-sdk-go/settings

We have to do that after we merge this PR tho

I think we can remove it and make the new one required before merging this - let us know

@s4kh s4kh force-pushed the chore-bump-go-version branch 3 times, most recently from 453d53a to 3cefb3b Compare October 15, 2024 16:30
@s4kh
Copy link
Contributor Author

s4kh commented Oct 15, 2024

I think we can remove it and make the new one required before merging this - let us know

Do we set the required option in the GitHub settings? I'll disable the drone CI in https://drone.grafana.net/grafana/grafana-plugin-sdk-go/settings.

env:
GOLANGCI_LINT_VERSION: v1.61.0

jobs:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a problem using one job for lint, build and test? Now it creates one check per job on the PR, would simplify to only have one job with multiple steps when configuring branch protection rules, e.g. which checks to be required
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was recommended by the golangci-lint docs to use separate job - https://github.com/golangci/golangci-lint-action?tab=readme-ov-file#how-to-use

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Though it works in the single job.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm okay 🤔 Guess it depends on if you want to run lint in parallel with build/test vs all three synchronously. No strong opinions really, it was more convenient for having one required check and configuring the branch protection

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Merged into single job.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ready to rumble. ✔️

@s4kh s4kh force-pushed the chore-bump-go-version branch from fe5a3f1 to 306e09e Compare October 15, 2024 16:46
@s4kh s4kh force-pushed the chore-bump-go-version branch from 306e09e to 52c1243 Compare October 15, 2024 16:49
@s4kh
Copy link
Contributor Author

s4kh commented Oct 15, 2024

@marefr for development have you configured golangci-lint on your machine and configured with your editor? I think it is better to lint errors(same lint config) in the editor and fix it before push.

@s4kh s4kh enabled auto-merge (squash) October 15, 2024 16:56
Copy link
Contributor

@marefr marefr left a comment

Choose a reason for hiding this comment

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

LGTM I modified the branch protection rules

@s4kh s4kh merged commit dc79381 into main Oct 15, 2024
4 checks passed
@s4kh s4kh deleted the chore-bump-go-version branch October 15, 2024 17:23
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.

Drop support for Go 1.21 Build: Migrate to GitHub Actions from Drone
5 participants