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

Use golang 1.18 #786

Merged
merged 1 commit into from
Apr 8, 2022
Merged

Use golang 1.18 #786

merged 1 commit into from
Apr 8, 2022

Conversation

pavolloffay
Copy link
Member

Signed-off-by: Pavol Loffay p.loffay@gmail.com

Replaces #784

@pavolloffay pavolloffay requested a review from a team March 17, 2022 15:34
@pavolloffay
Copy link
Member Author

The build failed on: golangci/golangci-lint#2649

@@ -35,6 +35,7 @@ jobs:
- name: Set up Go
uses: actions/setup-go@v2
with:
# TODO switch to Golang 1.18 once https://github.com/golangci/golangci-lint/issues/2649 is fixed
go-version: 1.17
Copy link
Member Author

Choose a reason for hiding this comment

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

I was able to fix the issue by using temporarily go 1.17 for lint.

once this PR is merged I will create an issue in this repo to track this.

@@ -1,6 +1,6 @@
module github.com/otel-allocator

go 1.17
go 1.18
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this the minimum version that is required to compile? Iam asking because of:

at RH there is probably no go 1.18 image for productization. We have to make sure the project can be built with go 1.17 as well

Copy link
Member Author

Choose a reason for hiding this comment

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

I am able to build it with go 1.17 without issues (e.g. by changing go builder version in the dockerfile)

Copy link
Member

Choose a reason for hiding this comment

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

oh ok, but it would allow people to use 1.18 features, right? Would then make sense to keep it at 1.17 until we have a build image?

Copy link
Member Author

Choose a reason for hiding this comment

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

That is a valid argument. If somebody will use 1.18 features we will have to fork :/

Copy link
Member

Choose a reason for hiding this comment

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

1.18 features make MUCH sense for Kubernetes Operators, like the support for generics. Much of the code can be simplified with that, I presume. But we are not in a hurry to change to it, though.

Copy link
Contributor

@VineethReddy02 VineethReddy02 left a comment

Choose a reason for hiding this comment

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

LGTM!

Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
@pavolloffay
Copy link
Member Author

PR rebased.

With the latest version of the golang-ci our lint job passes fine without any workaround.

@yuriolisa
Copy link
Contributor

LGTM!

@jpkrohling jpkrohling merged commit acfec1c into open-telemetry:main Apr 8, 2022
ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
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.

5 participants