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

Cirrus: Use versionable IMAGE_SUFFIX #17166

Merged
merged 1 commit into from
Feb 2, 2023

Conversation

cevich
Copy link
Member

@cevich cevich commented Jan 19, 2023

Image content hasn't changed much, the biggest thing here is the $IMAGE_SUFFIX value. This new schema is also fully manageable by renovate. Allowing a tag-push to c/automation_images to create image update PRs in all repos automatically.

ref: containers/automation_images#247

Also, cleanup a few comments and remove a disused testing task.

Signed-off-by: Chris Evich cevich@redhat.com

Does this PR introduce a user-facing change?

None

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none labels Jan 19, 2023
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 27, 2023
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 30, 2023
@cevich cevich changed the title [WIP] Cirrus: Use versionable IMAGE_SUFFIX Cirrus: Use versionable IMAGE_SUFFIX Jan 30, 2023
@cevich cevich marked this pull request as ready for review January 30, 2023 18:24
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 30, 2023
@rhatdan
Copy link
Member

rhatdan commented Jan 31, 2023

LGTM

Image content hasn't changed much, the biggest thing here is the
$IMAGE_SUFFIX value. This new schema is also fully manageable by
renovate. Allowing a tag-push to c/automation_images to create image
update PRs in all repos automatically.

ref: containers/automation_images#247

Also, cleanup a few comments and remove a disused testing task.

Signed-off-by: Chris Evich <cevich@redhat.com>
@cevich
Copy link
Member Author

cevich commented Feb 1, 2023

force-push: Rebased + fixed very minor comment typo.

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.

LGTM, this is beautiful work, just one gripe.

Comment on lines +29 to +32
FEDORA_NAME: "fedora-37" ### c20230120t152650z-f37f36u2204
FEDORA_AARCH64_NAME: "${FEDORA_NAME}-aarch64" ### c20230120t152650z-f37f36u2204
PRIOR_FEDORA_NAME: "fedora-36" ### c20230120t152650z-f37f36u2204
#UBUNTU_NAME: "ubuntu-2204" ### c20230120t152650z-f37f36u2204
Copy link
Member

Choose a reason for hiding this comment

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

Are these nasty comments absolutely necessary? I've been reviewing this indirectly via #17305 and the constant changes in comment are distracting.

Copy link
Member Author

@cevich cevich Feb 2, 2023

Choose a reason for hiding this comment

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

Not absolutely necessary. They're there because there's no other (easy) way to associate a IMAGE_SUFFIX value with any of the *_NAME values. In other words, I want maintainers to notice quickly if renovate suggests an incompatible update - i.e. replacing F37 with F38.

However, the current rules treat everything after the - as a "compatibility" value, so (assuming that works) the comment is just a "CYA" value that gets updated with images. Hopefully prompting a maintainer to check - for example, yep, these images still cover "Fedora-36".

Maybe this is just a corner-case we can ignore though?

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'll endeavor to remove these comments when I come across them. Such as in #17305

Copy link
Member

Choose a reason for hiding this comment

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

SGTM

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 2, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cevich, edsantiago

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 2, 2023
@edsantiago
Copy link
Member

I don't understand what renovatebot has to do with changing an image to f38.... but I'll trust your judgment in this.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 2, 2023
@openshift-merge-robot openshift-merge-robot merged commit 90b18d2 into containers:main Feb 2, 2023
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 12, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants