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

Bump minimum Go version to 1.19 #313

Merged
merged 5 commits into from
Aug 21, 2023

Conversation

anoopcs9
Copy link
Collaborator

@anoopcs9 anoopcs9 commented Jul 31, 2023

@anoopcs9
Copy link
Collaborator Author

Not urgent, we could wait till Go 1.21 is released(may be in 2-3 weeks time).

phlogistonjohn
phlogistonjohn previously approved these changes Jul 31, 2023
Copy link
Collaborator

@phlogistonjohn phlogistonjohn left a comment

Choose a reason for hiding this comment

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

I'm perpetually perplexed by the precise meaning of the go version present in the go.mod file. I'm pretty positive that they're even planning to change it. Usually I prefer to see the build versions change but not the mod file. But I am not going to pester you about it.

@anoopcs9 anoopcs9 requested review from spuiuk and obnoxxx August 1, 2023 06:00
@anoopcs9
Copy link
Collaborator Author

anoopcs9 commented Aug 9, 2023

Not urgent, we could wait till Go 1.21 is released(may be in 2-3 weeks time).

Go 1.21 is out now.

Copy link
Collaborator

@obnoxxx obnoxxx left a comment

Choose a reason for hiding this comment

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

I think "build:" is not the appropriate componeb for both the commit and the PR since this PR changes both the build itself and the github workflow actions.

I think these two changes should be separated in two commits and maybe two dependent PRs ...

@anoopcs9 anoopcs9 changed the title build: Bump minimum Go version to 1.19 Bump minimum Go version to 1.19 Aug 15, 2023
@mergify mergify bot dismissed phlogistonjohn’s stale review August 15, 2023 13:20

Pull request has been modified.

phlogistonjohn
phlogistonjohn previously approved these changes Aug 15, 2023
Copy link
Collaborator

@phlogistonjohn phlogistonjohn left a comment

Choose a reason for hiding this comment

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

Approved.

I agree with @obnoxxx that the build commit probably should be two commits but it doesn't bother me enough to block the PR.

Unfortunately, we're now going to continue to play catch-up because Go 1.21 has been released, making 1.19 now out of support so we should follow up with updates to 1.20... at least for the CI and containers (hopefully not go.mod again)

@anoopcs9
Copy link
Collaborator Author

Approved.

I agree with @obnoxxx that the build commit probably should be two commits but it doesn't bother me enough to block the PR.

Updated once more.

@anoopcs9 anoopcs9 dismissed obnoxxx’s stale review August 15, 2023 13:52

Split into smaller commits and updated the PR title.

Copy link
Collaborator

@phlogistonjohn phlogistonjohn left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@dpulls
Copy link

dpulls bot commented Aug 16, 2023

🎉 All dependencies have been resolved !

@anoopcs9
Copy link
Collaborator Author

/retest centos-ci/sink-clustered/mini-k8s-latest

Copy link
Collaborator

@phlogistonjohn phlogistonjohn left a comment

Choose a reason for hiding this comment

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

LGTM

@phlogistonjohn
Copy link
Collaborator

@obnoxxx or @synarete -- a 2nd review please

@mergify mergify bot merged commit d28e94b into samba-in-kubernetes:master Aug 21, 2023
12 checks passed
@mergify mergify bot added the priority-review This PR deserves a look label Sep 5, 2023
@anoopcs9 anoopcs9 deleted the bump-go-1.19 branch November 30, 2023 05:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority-review This PR deserves a look
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants