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

build: pin oapi-gen until toolbox gets 1.21.13 #1352

Merged
merged 1 commit into from
Sep 25, 2024

Conversation

lzap
Copy link
Collaborator

@lzap lzap commented Sep 24, 2024

Unsure how to implement pinning effectively for vendored projects. What I usually do is manually go get those libraries in a prepare script or Makefile.

@croissanne
Copy link
Member

croissanne commented Sep 24, 2024

How about pinning it in the files which have the directive ?

git grep -l 'github.com/oapi-codegen/oapi-codegen/v2/cmd/oapi-codegen' | xargs sed -i 's|github.com/oapi-codegen/oapi-codegen/v2/cmd/oapi-codegen|github.com/oapi-codegen/oapi-codegen/v2/cmd/[email protected]|g'

or

git grep -l 'github.com/oapi-codegen/oapi-codegen/v2/cmd/oapi-codegen' | xargs sed -i 's|github.com/oapi-codegen/oapi-codegen/v2/cmd/oapi-codegen|github.com/oapi-codegen/oapi-codegen/v2/cmd/[email protected]|g'

@lzap
Copy link
Collaborator Author

lzap commented Sep 24, 2024

For the record the problem is auto-updating of dependencies:

+ /home/runner/go/bin/go1.21.9 generate -mod=mod ./...
go: finding module for package github.com/oapi-codegen/oapi-codegen/v2/cmd/oapi-codegen
go: downloading github.com/oapi-codegen/oapi-codegen/v2 v2.4.0
go: finding module for package github.com/oapi-codegen/oapi-codegen/v2/cmd/oapi-codegen
go: toolchain upgrade needed to resolve github.com/oapi-codegen/oapi-codegen/v2/cmd/oapi-codegen
go: github.com/oapi-codegen/oapi-codegen/[email protected] requires go >= 1.[21](https://github.com/osbuild/image-builder/actions/runs/11012206487/job/30577910838?pr=1350#step:5:22).13 (running go 1.21.9; ../../../go.mod sets toolchain go1.21.9)
internal/clients/composer/package.go:1: running "go": exit status 1
go: finding module for package github.com/oapi-codegen/oapi-codegen/v2/cmd/oapi-codegen
go: finding module for package github.com/oapi-codegen/oapi-codegen/v2/cmd/oapi-codegen
go: toolchain upgrade needed to resolve github.com/oapi-codegen/oapi-codegen/v2/cmd/oapi-codegen
go: github.com/oapi-codegen/oapi-codegen/[email protected] requires go >= 1.21.13 (running go 1.21.9; ../../../go.mod sets toolchain go1.21.9)
internal/clients/content_sources/package.go:1: running "go": exit status 1
go: finding module for package github.com/oapi-codegen/oapi-codegen/v2/cmd/oapi-codegen
go: finding module for package github.com/oapi-codegen/oapi-codegen/v2/cmd/oapi-codegen
go: toolchain upgrade needed to resolve github.com/oapi-codegen/oapi-codegen/v2/cmd/oapi-codegen
go: github.com/oapi-codegen/oapi-codegen/[email protected] requires go >= 1.21.13 (running go 1.21.9; ../../../go.mod sets toolchain go1.21.9)
internal/clients/provisioning/package.go:1: running "go": exit status 1
go: finding module for package github.com/oapi-codegen/oapi-codegen/v2/cmd/oapi-codegen
go: finding module for package github.com/oapi-codegen/oapi-codegen/v2/cmd/oapi-codegen
go: toolchain upgrade needed to resolve github.com/oapi-codegen/oapi-codegen/v2/cmd/oapi-codegen
go: github.com/oapi-codegen/oapi-codegen/[email protected] requires go >= 1.21.13 (running go 1.21.9; ../../../go.mod sets toolchain go1.21.9)
internal/clients/recommendations/package.go:1: running "go": exit status 1
go: finding module for package github.com/oapi-codegen/oapi-codegen/v2/cmd/oapi-codegen
go: finding module for package github.com/oapi-codegen/oapi-codegen/v2/cmd/oapi-codegen
go: toolchain upgrade needed to resolve github.com/oapi-codegen/oapi-codegen/v2/cmd/oapi-codegen
go: github.com/oapi-codegen/oapi-codegen/[email protected] requires go >= 1.21.13 (running go 1.21.9; ../../go.mod sets toolchain go1.21.9)
internal/v1/server.go:1: running "go": exit status 1

Asked on Slack and got few recommendations and an article link which I am going to read in a bit. A recommendation was to use GOTOOLCHAIN=auto option. Need to learn about this.

@lzap
Copy link
Collaborator Author

lzap commented Sep 24, 2024

Slightly tuned the sed hack but this will do I guess.

Copy link

codecov bot commented Sep 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Files with missing lines Coverage Δ
internal/v1/server.go 76.69% <ø> (ø)

@lzap lzap force-pushed the pin-oapi branch 2 times, most recently from 4829b69 to 67b8199 Compare September 24, 2024 13:23
@lzap
Copy link
Collaborator Author

lzap commented Sep 24, 2024

Okay messed up sed, should be correct this time.

@@ -12,7 +13,8 @@ $GO_BINARY download
# Ensure dev tools are installed
which goimports || $GO_BINARY install golang.org/x/tools/cmd/goimports@latest

# Ensure that all code has been regenerated from its sources
# Ensure that all code has been regenerated from its sources with the pinned oapi version
git grep -l "github.com/oapi-codegen/oapi-codegen/v2/cmd/[email protected]" | grep -v prepare-source.sh | xargs sed -i "s|github.com/oapi-codegen/oapi-codegen/v2/cmd/oapi-codegen[[email protected]]*|github.com/oapi-codegen/oapi-codegen/v2/cmd/oapi-codegen@v$OAPI_VERSION|g"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

LOL keeps modifying this file too.

Bumps the go-deps group with 1 update: [github.com/osbuild/images](https://github.com/osbuild/images).

Updates `github.com/osbuild/images` from 0.87.0 to 0.88.0
- [Release notes](https://github.com/osbuild/images/releases)
- [Commits](osbuild/images@v0.87.0...v0.88.0)

---
updated-dependencies:
- dependency-name: github.com/osbuild/images
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: go-deps
...

Signed-off-by: dependabot[bot] <[email protected]>
@ondrejbudai
Copy link
Member

ondrejbudai commented Sep 24, 2024

dumb question: Why cannot we use Go 1.21.13?

@ondrejbudai
Copy link
Member

ondrejbudai commented Sep 24, 2024

Ah, is that because registry.access.redhat.com/ubi9/go-toolset has not been rebuilt yet? Go 1.21.13 is already in repositories, so we should be able to just dnf upgrade -y golang in the container. (it's needed to do some USER dancing because the default user is not root)

@ezr-ondrej
Copy link
Collaborator

dumb question: Why cannot we use Go 1.21.13?

#1349 failed to do it easily

@ezr-ondrej
Copy link
Collaborator

Looks 🍏 to me 🤷

@lzap
Copy link
Collaborator Author

lzap commented Sep 24, 2024

Exactly, the thing is some go tools now tries to upgrade to latest version which is causing the issue. Namely go generate here but it is a common problem across tools from 1.21+.

Tests are green.

@ezr-ondrej
Copy link
Collaborator

I see no objections, lets get it in :)

@ezr-ondrej ezr-ondrej enabled auto-merge (rebase) September 25, 2024 07:27
@ezr-ondrej ezr-ondrej merged commit f488bf0 into osbuild:main Sep 25, 2024
13 checks passed
@lzap lzap deleted the pin-oapi branch September 25, 2024 08:21
jamietanna added a commit to oapi-codegen/oapi-codegen that referenced this pull request Sep 28, 2024
Originally added in f77bd7b, we wanted
to pin the version to ensure that we were targeting the latest version
of Go 1.21.

However, there's no real value here, and as noted in [0] it can be
causing issues for some users.

There's nothing explicitly in Go 1.21.x that we need to pin to, just
that we need support for Go 1.21, so we can use Speakeasy's
`openapi-overlay` library.

Therefore we can relax the patch-version requirement, which will
alleviate issues for consumers.

[0]: osbuild/image-builder#1352
jamietanna added a commit to oapi-codegen/oapi-codegen that referenced this pull request Sep 28, 2024
Originally added in f77bd7b, we wanted
to pin the version to ensure that we were targeting the latest version
of Go 1.21.

However, there's no real value here, and as noted in [0] it can be
causing issues for some users.

There's nothing explicitly in Go 1.21.x that we need to pin to, just
that we need support for Go 1.21, so we can use Speakeasy's
`openapi-overlay` library.

Therefore we can relax the patch-version requirement, which will
alleviate issues for consumers.

This makes sure that we have _at a minimum_ a version of Go 1.21, and
makes `go mod tidy` happy.

[0]: osbuild/image-builder#1352
jamietanna added a commit to oapi-codegen/oapi-codegen that referenced this pull request Sep 28, 2024
Originally added in f77bd7b, we wanted
to pin the version to ensure that we were targeting the latest version
of Go 1.21.

However, there's no real value here, and as noted in [0] it can be
causing issues for some users.

There's nothing explicitly in Go 1.21.x that we need to pin to, just
that we need support for Go 1.21, so we can use Speakeasy's
`openapi-overlay` library.

Therefore we can relax the patch-version requirement, which will
alleviate issues for consumers.

This makes sure that we have _at a minimum_ a version of Go 1.21, and
makes `go mod tidy` happy.

[0]: osbuild/image-builder#1352
jamietanna added a commit to oapi-codegen/oapi-codegen that referenced this pull request Sep 28, 2024
Originally added in f77bd7b, we wanted
to pin the version to ensure that we were targeting the latest version
of Go 1.21.

However, there's no real value here, and as noted in [0] it can be
causing issues for some users.

There's nothing explicitly in Go 1.21.x that we need to pin to, just
that we need support for Go 1.21, so we can use Speakeasy's
`openapi-overlay` library.

Therefore we can relax the patch-version requirement, which will
alleviate issues for consumers.

This makes sure that we have _at a minimum_ a version of Go 1.21, and
makes `go mod tidy` happy.

[0]: osbuild/image-builder#1352
@jamietanna
Copy link

Apologies for this! https://github.com/oapi-codegen/oapi-codegen/releases/tag/v2.4.1 should now resolve this for y'all 🤞

@lzap
Copy link
Collaborator Author

lzap commented Sep 29, 2024

Jeeeez Jamie, no need to apologize. Thank you for your help on the Go Slack channel and for your hard work on this project, and many more!

Let’s revert the hack (or keep it): #1355

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