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: Upgrade Go version to 1.19.1 (backport) #55733

Merged
merged 8 commits into from Sep 28, 2022

Conversation

sakjur
Copy link
Contributor

@sakjur sakjur commented Sep 26, 2022

What this PR does / why we need it:

Upgrades the version of Go used to build Grafana v9.1.x to 1.19.1.

@github-actions
Copy link
Contributor

Backend code coverage report for PR #55733

Plugin Main PR Difference
azuremonitor 68.3% 68.3% 0%
cloudmonitoring 57.7% 57.7% 0%

@github-actions
Copy link
Contributor

Frontend code coverage report for PR #55733
No changes

@grafanabot
Copy link
Contributor

@sakjur sakjur added this to the 9.1.7 milestone Sep 26, 2022
@sakjur sakjur marked this pull request as ready for review September 26, 2022 11:33
@sakjur sakjur requested review from a team as code owners September 26, 2022 11:33
@sakjur sakjur requested review from a team September 26, 2022 11:33
@sakjur sakjur requested review from a team as code owners September 26, 2022 11:33
@sakjur sakjur requested a review from a team September 26, 2022 11:33
@sakjur sakjur requested review from a team, andresmgot, aangelisc, idafurjes, papagian and ying-jeanne and removed request for ying-jeanne, a team, andresmgot and aangelisc September 26, 2022 11:34
Copy link
Contributor

@papagian papagian 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 have spotted some minor issues with the swagger generation.

pkg/api/quota.go Outdated Show resolved Hide resolved
pkg/api/api.go Outdated
Comment on lines 24 to 26
//
// type: basic
//
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be valid yaml otherwise the make swagger-api-spec fails with:

yaml: unmarshal errors:
  line 4: mapping key "type" already defined at line 2
Suggested change
//
// type: basic
//
// type: basic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

go fmt won't allow that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can fix it by making all lines in the yaml indented, but then I get

SWAGGER_GENERATE_EXTENSION=false /Users/sakjur/go/bin/swagger-v0.30.2 generate spec -m -w pkg/server -o public/api-spec.json \
	-x "github.com/grafana/grafana/pkg/services/ngalert/api/tooling/definitions" \
	-x "github.com/prometheus/alertmanager" \
	-i pkg/api/swagger_tags.json
yaml: line 2: mapping values are not allowed in this context
make: *** [--swagger-api-spec] Error 1

We can't really not upgrade, fwiw, so unless we'll find a good solution to making Swagger work for v9.1.x, I'd suggest breaking Swagger for the v9.1.x branch (it should be finalized for that version around the time of v9.1.0, so it wouldn't be the weirdest of things to do)

Copy link
Contributor

@papagian papagian Sep 26, 2022

Choose a reason for hiding this comment

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

I'm afraid breaking swagger is not an option unless build-go target no more depends on that

Copy link
Contributor

Choose a reason for hiding this comment

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

this is how it's in main and seems to be OK both with gofmt and swagger
https://github.com/grafana/grafana/blob/main/pkg/api/api.go#L1-L30

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, main has a tab before basic as well, it's not just this line that's changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this now, but I'm still getting the yaml: line 2: mapping values are not allowed in this context error and I don't get how to track it down?

Copy link
Contributor

Choose a reason for hiding this comment

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

I have pushed some changes that work for me locally

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🙏 Thank you!

pkg/api/api.go Outdated Show resolved Hide resolved
@@ -126,7 +126,7 @@ RUN apt-get update && \
gcc \
g++ \
git \
jq \
jq \
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Heh, it's me aligning this, but missing that the first whitespace on the jq was a tab and not four spaces

Co-authored-by: Sofia Papagiannaki <1632407+papagian@users.noreply.github.com>
@grafanabot
Copy link
Contributor

@grafanabot
Copy link
Contributor

@grafanabot
Copy link
Contributor

@grafanabot
Copy link
Contributor

@grafanabot
Copy link
Contributor

@sakjur sakjur changed the title [v9.1.x] Chore: Upgrade Go version to 1.19.1 Chore: Upgrade Go version to 1.19.1 (backport) Sep 27, 2022
@sakjur sakjur merged commit 05b4ce4 into v9.1.x Sep 28, 2022
@sakjur sakjur deleted the emil/202209/9.1.x-go-1.19.1 branch September 28, 2022 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants