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

Specify full x.y.z version of Go in go.mod #1442

Merged
merged 1 commit into from
Dec 11, 2024

Conversation

grdryn
Copy link
Member

@grdryn grdryn commented Dec 11, 2024


JIRA: https://issues.redhat.com/browse/RHOAIENG-16819

Description

It seems like on some platforms, when the local installation
installation of Go decides that it needs to download a different
toolchain, that it may fail unless we either specify a full x.y.z
version in the go directive, or explicitly specify the toolchain
directive.

I believe for our uses, using a fully specified x.y.z go directive
value is preferable, to prevent any issues with explicitly defining a
toolchain where we might have a newer version in a container image
build that has a default of GOTOOLCHAIN='local' (I'm not 100% sure
if that would cause any issues, but I'm 100% sure that the way
proposed in this commit shouldn't).

How Has This Been Tested?

  • builds fine
  • runs fine

Screenshot or short clip

Merge criteria

  • You have read the contributors guide.
  • Commit messages are meaningful - have a clear and concise summary and detailed explanation of what was changed and why.
  • Pull Request contains a description of the solution, a link to the JIRA issue, and to any dependent or related Pull Request.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

JIRA: https://issues.redhat.com/browse/RHOAIENG-16819

It seems like on some platforms, when the local installation
installation of Go decides that it needs to download a different
toolchain, that it may fail unless we either specify a full x.y.z
version in the `go` directive, or explicitly specify the `toolchain`
directive.

I believe for our uses, using a fully specified x.y.z `go` directive
value is preferable, to prevent any issues with explicitly defining a
toolchain where we might have a newer version in a container image
build that has a default of `GOTOOLCHAIN='local'` (I'm not 100% sure
if that would cause any issues, but I'm 100% sure that the way
proposed in this commit shouldn't).
@grdryn
Copy link
Member Author

grdryn commented Dec 11, 2024

/cc @VaishnaviHire

This is a follow up to #1439 after feedback in #1440 (comment)

@openshift-ci openshift-ci bot requested a review from VaishnaviHire December 11, 2024 13:25
@VaishnaviHire
Copy link
Member

/lgtm

needs rebase ;(

@ykaliuta
Copy link
Contributor

needs rebase ;(
does it?

Copy link

codecov bot commented Dec 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@5f456ad). Learn more about missing BASE report.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1442   +/-   ##
=======================================
  Coverage        ?   19.71%           
=======================================
  Files           ?      149           
  Lines           ?    10052           
  Branches        ?        0           
=======================================
  Hits            ?     1982           
  Misses          ?     7846           
  Partials        ?      224           

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

@grdryn
Copy link
Member Author

grdryn commented Dec 11, 2024

needs rebase ;(

I wonder if that was a GH blip, or a change with how it works? 🤔 I also saw that message that it was out of date with the base branch, but now it says the following for me, which seems to indicate that it'll get auto-merged fine once it gets the approved label:

image

Edit: I'm using the "new merge experience" (?new_mergebox=true), are you? Maybe that causes some confusion by warning that the branch needs to be updated until all of the checks pass, and then it instead changes to "actually it's fine".

@ykaliuta
Copy link
Contributor

needs rebase ;(

I wonder if that was a GH blip, or a change with how it works? 🤔

No idea, I checked the merge itself :)

Copy link
Member

@VaishnaviHire VaishnaviHire left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link

openshift-ci bot commented Dec 11, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: VaishnaviHire

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit 79e1b94 into opendatahub-io:main Dec 11, 2024
10 checks passed
@grdryn grdryn deleted the gomod-go-fix branch December 12, 2024 00:48
zdtsw pushed a commit to zdtsw-forking/opendatahub-operator that referenced this pull request Jan 2, 2025
JIRA: https://issues.redhat.com/browse/RHOAIENG-16819

It seems like on some platforms, when the local installation
installation of Go decides that it needs to download a different
toolchain, that it may fail unless we either specify a full x.y.z
version in the `go` directive, or explicitly specify the `toolchain`
directive.

I believe for our uses, using a fully specified x.y.z `go` directive
value is preferable, to prevent any issues with explicitly defining a
toolchain where we might have a newer version in a container image
build that has a default of `GOTOOLCHAIN='local'` (I'm not 100% sure
if that would cause any issues, but I'm 100% sure that the way
proposed in this commit shouldn't).

(cherry picked from commit 79e1b94)
@zdtsw zdtsw mentioned this pull request Jan 2, 2025
5 tasks
zdtsw added a commit that referenced this pull request Jan 2, 2025
* Update Dockerfiles to Go 1.22 (#1424)

JIRA: https://issues.redhat.com/browse/RHOAIENG-16819

The UBI8 go-toolset image that we use for building is already on 1.22,
so I don't think there's any reason to remain bound to 1.21.

Also, upstream Go only support the two most recent minor versions:
currently that means 1.23 and 1.22; 1.21 is no longer receiving any
updates from the Go developers.

* Update Go version in go.mod to 1.22 (Go 1.22: 2/3) (#1439)

* Update Dockerfiles to Go 1.22

JIRA: https://issues.redhat.com/browse/RHOAIENG-16819

The UBI8 go-toolset image that we use for building is already on 1.22,
so I don't think there's any reason to remain bound to 1.21.

Also, upstream Go only support the two most recent minor versions:
currently that means 1.23 and 1.22; 1.21 is no longer receiving any
updates from the Go developers.

* Update Go version in go.mod to 1.22

JIRA: https://issues.redhat.com/browse/RHOAIENG-16819

This just updates the Go version in the go directive in go.mod. The
code changes to add nolint suppression is temporary and those will be
addressed in a separate commit.

(cherry picked from commit 5f456ad)

* Specify full x.y.z version of Go in go.mod (#1442)

JIRA: https://issues.redhat.com/browse/RHOAIENG-16819

It seems like on some platforms, when the local installation
installation of Go decides that it needs to download a different
toolchain, that it may fail unless we either specify a full x.y.z
version in the `go` directive, or explicitly specify the `toolchain`
directive.

I believe for our uses, using a fully specified x.y.z `go` directive
value is preferable, to prevent any issues with explicitly defining a
toolchain where we might have a newer version in a container image
build that has a default of `GOTOOLCHAIN='local'` (I'm not 100% sure
if that would cause any issues, but I'm 100% sure that the way
proposed in this commit shouldn't).

(cherry picked from commit 79e1b94)

* Address new linter errors introduced with Go 1.22 (#1440)

JIRA: https://issues.redhat.com/browse/RHOAIENG-16819
(cherry picked from commit e30eb81)

* fix: add nolint for loop

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* update: remove instead of skipping lint

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

---------

Signed-off-by: Wen Zhou <wenzhou@redhat.com>
Co-authored-by: Gerard Ryan <git@grdryn.xyz>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants