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

Adding support the CentOS based CoreOS images #503

Merged

Conversation

bradmwilliams
Copy link
Contributor

No description provided.

@bradmwilliams
Copy link
Contributor Author

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 9, 2023
@openshift-ci openshift-ci bot requested review from AlexNPavel and hoxhaeris March 9, 2023 13:57
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 9, 2023
Comment on lines +151 to +154
// TODO: This should hopefully only be temporary...
if m[4] == "92" {
return fmt.Sprintf("prod/streams/%s.%s-9.2", m[2], m[3]), true
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is gross and I hope it's only temporary...
@cgwalters and/or @jlebon Do you either of you know what the long term plan for how the release browser is going to handle new 9.2 CentOS based images?

Copy link
Member

Choose a reason for hiding this comment

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

Ouch. I think we should probably figure out how to have this code have access to more information so it doesn't have to do this kind of guessing. Does it have access to the RHCOS container image labels? We could put the pipeline stream name the build came from in there.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, embedding this information in the container image makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code is the thing nightmares are made of... the information we have is what we're parsing from the output of an oc adm release info --changelog command. 😢

Copy link
Member

Choose a reason for hiding this comment

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

How hard would it be to change that? Can the code do another call to e.g. skopeo on the RHCOS image? Can the oc adm release info output be augmented to also output labels of the referred images?

OK, I see --includes-images which does work but it's super expensive because it's querying the metadata for all the images. Maybe simplest is to add an --include-image so then it can do --include-image rhel-coreos to only add the info for that one?

I also see annotations in the default output like this:

        {
          "name": "rhel-coreos-8",
          "annotations": {
            "io.openshift.build.commit.id": "",
            "io.openshift.build.commit.ref": "",
            "io.openshift.build.source-location": ""
          },
          "from": {
            "kind": "DockerImage",
            "name": "quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:65f00f7a151a6a393f2d12a504f5ae14e1e7a249a35db23b0d053466d6469cad"
          },
          "generation": null,
          "importPolicy": {},
          "referencePolicy": {
            "type": ""
          }
        },

Not sure how those get there. They should be filled with info from https://github.com/openshift/os, but maybe we can also enhance it so it includes additional annotations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could certainly add some logic to extract the information if/when it exists. Something like this should be sufficient and not too time consuming:

$ oc adm release info --image-for=rhel-coreos registry.ci.openshift.org/ocp/release:4.14.0-0.nightly-2023-03-08-194110
quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:cd299b2bf3cc98fb70907f152b4281633064fe33527b5d6a42ddc418ff00eec1
$ oc image info quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:cd299b2bf3cc98fb70907f152b4281633064fe33527b5d6a42ddc418ff00eec1

Copy link
Member

Choose a reason for hiding this comment

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

@bradmwilliams
Copy link
Contributor Author

/retest

1 similar comment
@bradmwilliams
Copy link
Contributor Author

/retest

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 13, 2023

@bradmwilliams: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@bradmwilliams
Copy link
Contributor Author

/unhold
We're going to run with this for now.

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 13, 2023
@jupierce
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 13, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 13, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bradmwilliams, jupierce

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:
  • OWNERS [bradmwilliams,jupierce]

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

@openshift-merge-robot openshift-merge-robot merged commit 6229a12 into openshift:master Apr 13, 2023
@bradmwilliams bradmwilliams deleted the coreos-bump-fixes branch April 13, 2023 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants