Skip to content

Conversation

@baude
Copy link
Member

@baude baude commented Mar 10, 2022

Signed-off-by: Brent Baude [email protected]

@baude baude changed the title Add Fedora-36 native compile [WIP]Add Fedora-36 native compile Mar 10, 2022
@baude baude force-pushed the addf36nativebuild branch 4 times, most recently from e75d8b0 to d494fa8 Compare March 10, 2022 20:00
@baude baude changed the title [WIP]Add Fedora-36 native compile Add Fedora-36 native compile Mar 10, 2022
Copy link
Collaborator

@flouthoc flouthoc 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
Member

@lsm5 lsm5 left a comment

Choose a reason for hiding this comment

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

LGTM. @cevich PTAL.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 11, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: baude, flouthoc, lsm5

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [baude,flouthoc,lsm5]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link
Member

@edsantiago edsantiago left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, but two maintenance concerns

container:
cpu: 2
memory: 2
image: quay.io/libpod/fedora36rust
Copy link
Member

Choose a reason for hiding this comment

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

II get super nervous around :latest, and even more so when it's in an environment that needs to be maintained over time. Could I humbly request that you tag this image (in quay) with a suitable YYYYMMDD and use that tag here?

Copy link
Member Author

Choose a reason for hiding this comment

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

what is to be nervous about? this is supposed to mimic actual distribution builds so we don't incur problems when it comes to packaging. should it not be latest? that is the one that gets update. tell me more

Copy link
Member

Choose a reason for hiding this comment

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

that is the one that gets update

And that's the crux. Said updates are out-of-band wrt the code that relies on it (here in CI). We've seen this happen several times: foo.image:latest gets updated, and a few days later something breaks on a maintenance branch because that update changed a behavior that nobody even knew the old branch relied on. See, e.g., containers/podman#12343

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we actually want that in the case though, no? If the distribution ships an updated cargo, then we want to be testing with that on all branches, as we are pegged to the distribution level here; not the cargo version. What so you?

Copy link
Member

Choose a reason for hiding this comment

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

It's a tradeoff. It's one that we make often, and sometimes it's the right call, sometimes it's not.

I'm not blocking the PR, merely calling attention to it. If this is a deliberate and well-thought-out choice, then so be it. But could I at least request that you give the existing image a secondary descriptive tag? That way if/when there's a SNAFU with an image upgrade, we can do an emergency revert.

@baude baude force-pushed the addf36nativebuild branch from d494fa8 to e5bd535 Compare March 14, 2022 19:55
@Luap99
Copy link
Member

Luap99 commented Mar 14, 2022

/lgtm
/hold

Signed-off-by: Brent Baude <[email protected]>
@baude baude force-pushed the addf36nativebuild branch from e5bd535 to e14f59c Compare March 14, 2022 20:00
@openshift-ci openshift-ci bot removed the lgtm label Mar 14, 2022
@Luap99
Copy link
Member

Luap99 commented Mar 14, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Mar 14, 2022
@edsantiago
Copy link
Member

Duplication Is Evil!

/lgtm
/hold

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 14, 2022

@edsantiago: changing LGTM is restricted to collaborators

Details

In response to this:

Duplication Is Evil!

/lgtm
/hold

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@baude
Copy link
Member Author

baude commented Mar 14, 2022

/hold cancel

@openshift-merge-robot openshift-merge-robot merged commit 89030ae into containers:main Mar 14, 2022
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.

6 participants