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

Update Go version to 1.20 #171

Merged
merged 1 commit into from
Aug 1, 2023
Merged

Conversation

barrettj12
Copy link
Contributor

@barrettj12 barrettj12 commented Dec 8, 2022

Go 1.14 has been end-of-life for years, and now we're running into problems like not supporting io.ReadAll. Time to update it.

In our GitHub CI tests, I upgraded actions/setup-go to v3, and now we can get the Go version from go.mod.

.github/workflows/tests.yml Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
Copy link
Contributor

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

Per discussion, I'm okay with this update (though I have a question below about testing on two versions). But it'd be good to run this past @niemeyer as there are other teams that use Pebble now.

.github/workflows/tests.yml Outdated Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
Copy link
Contributor

@niemeyer niemeyer left a comment

Choose a reason for hiding this comment

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

Thanks or the PR, Jordan. A few questions below.

.github/workflows/tests.yml Outdated Show resolved Hide resolved
@jnsgruk
Copy link
Member

jnsgruk commented Aug 1, 2023

@benhoyt @hpidcock do we need to revisit this, or should we just close the PR?

@barrettj12
Copy link
Contributor Author

Considering we're now shipping Pebble binaries (which are built with Go 1.20 as well), I don't know if we still need to support old Go versions?

- [ci] update actions/setup-go to v3
- [ci] get go version from go.mod
@jnsgruk
Copy link
Member

jnsgruk commented Aug 1, 2023

I spoke with @niemeyer today about this PR. It looks like Juju is now building using 1.20 across all supported branches, so we shouldn't have a problem moving to 1.20 here - I've updated the branch to include the bump to 1.20 where it was previously 1.19 and rebased on master.

@barrettj12 Before we consider making a change like this, we need to make sure that we're not going to affect any of our downstream consumers, which I think is the piece that was missing from your original PR :)

So...

  • @flotter - can you confirm that this isn't going to impact your projects?
  • @cjdcordeiro or @tigarmo - can you confirm that we're okay from a Rockcraft perspective? My guess is we're fine since Pebble is sourced from the published snap
  • @hpidcock can you confirm we're good from a Juju perspective here?

If we're all good on those fronts, then we can go ahead and merge this -- we're fine to live with a deprecation warning if we need to - the features won't go away until go2 in all likelihood - and we can reassess at that point.

@hpidcock / @benhoyt - feel free to merge once we get confirmation from the others :)

@flotter
Copy link
Contributor

flotter commented Aug 1, 2023

@jnsgruk @barrettj12 Our project selected Go 1.20 as the starting point, so I think this proposal aligns well with us. Thanks for checking.

Should we update the PR title to reflect Go 1.20 (I assume this is the latest proposal)?

@flotter flotter self-requested a review August 1, 2023 13:47
@jnsgruk jnsgruk changed the title Update Go version to 1.19 Update Go version to 1.20 Aug 1, 2023
.github/workflows/tests.yml Show resolved Hide resolved
@tigarmo
Copy link

tigarmo commented Aug 1, 2023

@jnsgruk as far as Rockcraft goes, we should be fine - even before we moved to the snap we used go 1.19 to build locally.

@jnsgruk jnsgruk merged commit 6991b3c into canonical:master Aug 1, 2023
16 checks passed
@benhoyt benhoyt deleted the update-go branch August 1, 2023 20:22
@cjdcordeiro
Copy link
Collaborator

cjdcordeiro commented Aug 2, 2023

@jnsgruk as far as Rockcraft goes, we should be fine - even before we moved to the snap we used go 1.19 to build locally.

💯

https://github.com/canonical/rockcraft/blob/d81be44494fea5fd16f4d4baf2a6cafa66bfb083/rockcraft/pebble.py#L32-L36


The latest stable Go snap is also 1.20, which is what we use to build the Pebble snap:

build-snaps:
- go

Maybe it'll be good to pinpoint that Go snap version in Pebble's snapcraft.yaml -> #271

@sparkiegeek
Copy link
Contributor

I spoke with @niemeyer today about this PR. It looks like Juju is now building using 1.20 across all supported branches, so we shouldn't have a problem moving to 1.20 here - I've updated the branch to include the bump to 1.20 where it was previously 1.19 and rebased on master.

@barrettj12 Before we consider making a change like this, we need to make sure that we're not going to affect any of our downstream consumers, which I think is the piece that was missing from your original PR :)

So...

* @flotter - can you confirm that this isn't going to impact your projects?

* @cjdcordeiro or @tigarmo - can you confirm that we're okay from a Rockcraft perspective? My guess is we're fine since Pebble is sourced from the published snap

* @hpidcock can you confirm we're good from a Juju perspective here?

If we're all good on those fronts, then we can go ahead and merge this -- we're fine to live with a deprecation warning if we need to - the features won't go away until go2 in all likelihood - and we can reassess at that point.

@hpidcock / @benhoyt - feel free to merge once we get confirmation from the others :)

You forgot about us!

We use golang-go package from Jammy to build Pebble, which is 1.18 AFAICS

@jnsgruk
Copy link
Member

jnsgruk commented Aug 2, 2023

Sorry about that @sparkiegeek! You'll be on the list for next time, for sure.

Adam and I spoke briefly offline, and they can quite easily move to using the go snap for building, so this won't materialise as much of a problem.

@benhoyt benhoyt mentioned this pull request Sep 1, 2023
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.

10 participants