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

Vendor in latest containers/(storage, image) #2083

Merged
merged 1 commit into from
Jul 12, 2024

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Jul 10, 2024

Making sure we test containers/storage and image changes ASAP.

Copy link

We were not able to find or create Copr project packit/containers-common-2083 specified in the config with the following error:

Cannot create a new Copr project (owner=packit project=containers-common-2083 chroots=['centos-stream-9-x86_64', 'centos-stream-10-x86_64']): Response is not in JSON format, there is probably a bug in the API code.

Please check your configuration for:

  1. typos in owner and project name (groups need to be prefixed with @)
  2. whether the project name doesn't contain not allowed characters (only letters, digits, underscores, dashes and dots must be used)
  3. whether the project itself exists (Packit creates projects only in its own namespace)
  4. whether Packit is allowed to build in your Copr project
  5. whether your Copr project/group is not private

@rhatdan
Copy link
Member Author

rhatdan commented Jul 11, 2024

go.mod Outdated

// Warning: Ensure the "go" and "toolchain" versions match exactly to prevent unwanted auto-updates
toolchain go1.21.0
toolchain go1.22.5
Copy link
Member

Choose a reason for hiding this comment

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

This not correct please revert that change

Copy link
Member Author

Choose a reason for hiding this comment

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

make vendor-in-container is doing this automatically now.

Copy link
Member Author

Choose a reason for hiding this comment

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

The latest make vendor-in-container removed those lines. Is this also a problem? Do we need to fix vendor-in-container to not remove them?

Copy link
Contributor

Choose a reason for hiding this comment

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

go mod … bumps toolchain to whatever you are running when it bumps go for any other reason.

go.mod Outdated
Comment on lines 5 to 6
// Warning: Ensure the "go" and "toolchain" versions match exactly to prevent unwanted auto-updates
toolchain go1.21.0
Copy link
Member

Choose a reason for hiding this comment

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

This should stay so we have the comment to remind us to never bump this further than the minimum go version.

Copy link
Member Author

Choose a reason for hiding this comment

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

Then we need to fix the tools built into the Makefile to not remove the line or re-add it when it gets removed. Very easy to remove this line.

@Luap99
Copy link
Member

Luap99 commented Jul 11, 2024

What go version is used here, 1.22? Looks like they changed the behaviour regarding this toolchain line.
cc @mtrmac @cevich

@Luap99
Copy link
Member

Luap99 commented Jul 11, 2024

I guess we need to drop the toolchain line then if there is no other way to make golang module management happy.
But let's keep a comment in there then that we never want a toolchain line newer than the minimum go version listed there.

@rhatdan
Copy link
Member Author

rhatdan commented Jul 11, 2024

go version
go version go1.22.5 linux/amd64

@mtrmac
Copy link
Contributor

mtrmac commented Jul 11, 2024

What go version is used here, 1.22? Looks like they changed the behaviour regarding this toolchain line. cc @mtrmac @cevich

There are two things going on:

  • go 1.21 means “any version of the 1.21 language and standard library including pre-releases”, while go 1.21.0 means “the standard library at least as of go 1.21.0. And some dependency updated from go 1.21togo 1.21.0, which propagates here as well. And, as always, triggering a goupdate causes atoolchain $whatIAmRunning` to be added.
  • go X implies toolchain X; so, now that we have go 1.21.0, toolchain 1.21.0 is considered redundant and removed.

Neither of these directly change our goals and design: on any go bump, we will probably have to manually update toolchain down again.


It does, though, show another way for the motivating problem in our design might hypothetically show up: any dependency can, at any time, decide that it depends on go 1.21.555 because .555 has fixed some vulnerability — and the module graph would propagate that and not give us a way to opt out and to build with .554; so we could lose control of the toolchain requirement anyway. (It seems that a replace go directive might work in this situation. Let’s hope it doesn’t come to that)


But I agree that the comment should stay, or maybe be updated to

// Warning: If a “toolchain” directive exists, ensure the version is set _exactly_ to the version in "go" to prevent unwanted auto-updates

@rhatdan
Copy link
Member Author

rhatdan commented Jul 11, 2024

Go itself is removing the warning and changing the version. The only editing of go.mod is around the version number of containers/storage and containers/image.

make vendor-in-container does the rest.

@cevich
Copy link
Member

cevich commented Jul 11, 2024

Reminder: We still have a 1.21 constraint for proposed updates in place. Should that be bumped up to 1.22.0?

@mtrmac
Copy link
Contributor

mtrmac commented Jul 11, 2024

I read https://bodhi.fedoraproject.org/updates/?packages=golang to mean that we can’t, basically until end of the year, without abandoning F39.

@mtrmac
Copy link
Contributor

mtrmac commented Jul 11, 2024

Go itself is removing the warning and changing the version.

I see that when updating go.

But after the comment + toolchain is removed, if I add a comment to the ~same place, go mod tidy preserves it for me.

@Luap99
Copy link
Member

Luap99 commented Jul 12, 2024

Yeah I think you can add the comment after the line was removed. Also I guess we can add the comment above the go line which should then be preserved.

Making sure we test containers/storage and image changes ASAP.

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
Copy link
Member

@Luap99 Luap99 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
Contributor

openshift-ci bot commented Jul 12, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99, rhatdan

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

@rhatdan rhatdan added the lgtm label Jul 12, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 237a317 into containers:main Jul 12, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants