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: downgrade minimum Go version in go.mod #6318

Merged
merged 2 commits into from
May 15, 2024
Merged

Conversation

mohammed90
Copy link
Member

Just like caddyserver/certmagic#289, the dependency of caddyserver/zerossl forced Caddy to require go1.22. None of them have a hard dependency on the language semantics of 1.22, as far as I know. It's causing pains when building custom Caddy using the builder image variant. It's best to push out the new builder variant with the newer version of Go before upgrading the minimum required version here.

This PR is only ready once caddyserver/certmagic#289 is merged. Then we can only validate our language semantics need and (most likely) merge this PR.

Depends on caddyserver/certmagic#289

@mholt mholt marked this pull request as ready for review May 15, 2024 19:23
Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Thanks for helping with this!

(For whatever it's worth: I only just learned that as of Go 1.21, the go directive in go.mod is a minimum required Go version, and that is a new breaking change in the Go toolchain.)

@mholt mholt enabled auto-merge (squash) May 15, 2024 19:25
@mholt mholt merged commit 4486048 into master May 15, 2024
22 of 23 checks passed
@mholt mholt deleted the downgrade-min-go branch May 15, 2024 19:28
@mholt
Copy link
Member

mholt commented May 15, 2024

This is weird, the tests on IBM Z are failing consistently with this change.

I have no idea why automerge happened because that test is failing. (Oh, maybe it's not required to pass before merging?)

@francislavoie
Copy link
Member

We have go version go1.22.3 linux/s390x on that machine. That shouldn't be a problem 🤔

@francislavoie
Copy link
Member

I re-ran the job another time, it passed this time https://github.com/caddyserver/caddy/actions/runs/9101519695/job/25020869066

@mholt
Copy link
Member

mholt commented May 15, 2024

GitHub needs a 🤷 emoji reaction

@brotbert
Copy link

@mholt

(For whatever it's worth: I only just learned that as of Go 1.21, the go directive in go.mod is a minimum required Go version, and that is a new breaking change in the Go toolchain.)

But you did it again (now in caddy-webdav): mholt/caddy-webdav@bfcd390 😬

@francislavoie
Copy link
Member

You're right @brotbert, opened mholt/caddy-webdav#43 to solve it.

@mholt
Copy link
Member

mholt commented May 22, 2024

Oh, wait... but I didn't change that. I mean, clearly I did, but I did not edit those lines.

I wonder if this was done by my editor on my laptop where I have Go 1.22 installed and I used the "upgrade transitive dependencies" feature.

Sorry I didn't see that. How surprising and unfortunate 😕

@mohammed90
Copy link
Member Author

mohammed90 commented May 22, 2024

Oh, wait... but I didn't change that. I mean, clearly I did, but I did not edit those lines.

I wonder if this was done by my editor on my laptop where I have Go 1.22 installed and I used the "upgrade transitive dependencies" feature.

Sorry I didn't see that. How surprising and unfortunate 😕

You upgraded a dependency whose new version requires go1.22, which forced the edit of that line in go.mod. it's not because you have go1.22 installed.

@mholt
Copy link
Member

mholt commented May 22, 2024

This still seems like a problem though. If we can't upgrade dependencies then how do we upgrade dependencies?

@francislavoie
Copy link
Member

francislavoie commented May 22, 2024

You just upgrade the one dependency you need to upgrade by hand (edit go.mod), then run go mod tidy. That's all. Don't run go get -u or w/e.

But only upgrade deps if necessary. Don't bump the Caddy version unless necessary (i.e. using a new API, or using an API that changed), and don't use new APIs before the stable release of Caddy otherwise anyone on the current stable won't be able to build anymore (unless they target the previous tag/commit, but majority of users won't, they'll try to use latest).

@mohammed90
Copy link
Member Author

mohammed90 commented May 22, 2024

Think of it this way:
When you contract Company A for some work who sub-contracts some of its work to Company B, you wouldn't go to Company B directly to change their terms with Company A behind Company A's back.

@mholt
Copy link
Member

mholt commented May 22, 2024

It's just weird that the editor does that automatically when it knows there's not a func main() in it.

We will just have to accept plugins raising all the alarm bells of old versions from naive vulnerability scanners.

@EdenSpire
Copy link

I'm happy everything is sorted.
I spent hours to find the culprit.

Thanks for fast response guys!!

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