Skip to content

Conversation

@jlebon
Copy link
Member

@jlebon jlebon commented Jan 28, 2025

This basically fixes the gap that the infobox added in #648 warns about. Instead of relying on the CoreOS release browser differ, we directly query for the diff using oc adm release info, which recently learned to do this, and render it at the bottom of the page.

This removes the VPN requirement on being able to see the RPM diff.

In the case of the new layered node images, replace the diff link to instead link to this new rendered section. We could in theory always link to that, but let's be conservative. This provide a natural rollout of the feature in case it needs ironing out.

Implementation-wise, we don't want to block the main changelog render on getting the RPM diff. So we structure this the same way as the changelog render itself; if the oc command takes too long, we display a nice message to check again later. Once the command finishes and the output is cached, we include it in the following render requests.


Sample renders

When diff still loading

image

When diff is loaded

image

@jlebon
Copy link
Member Author

jlebon commented Jan 28, 2025

Requires: openshift/oc#1966

@bradmwilliams
Copy link
Collaborator

/approve
Only real concern I have here is that this functionality is buried at the bottom of the changelog, which can be massive, and I'm not really sure if anyone ever gets/goes that far down the page.

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 29, 2025
@bradmwilliams
Copy link
Collaborator

/hold
Because the requires openshift/oc#1966, we'll need to bump our tools bootstrap version to pick it up before this can safely merge.

@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 Jan 29, 2025
@jlebon
Copy link
Member Author

jlebon commented Jan 29, 2025

/approve Only real concern I have here is that this functionality is buried at the bottom of the changelog, which can be massive, and I'm not really sure if anyone ever gets/goes that far down the page.

Yeah, wasn't sure where best to put it. The other place I was thinking of was right after the Components section, but within a <details> tag that users can expand if they're interested. That way it doesn't take up much real estate by default. But it does mean more open-heart surgery on the Markdown/HTML changelog text that gets generated. Adding it at the end allowed me to sidestep all that.

Though I think also taking over the diff link does alleviate this concern a bit. Anyone interested in the diff specifically can click that link and go to the bottom of the page directly.

@bradmwilliams
Copy link
Collaborator

Though I think also taking over the diff link does alleviate this concern a bit. Anyone interested in the diff specifically can click that link and go to the bottom of the page directly.

That works! Anything that prevents having to perform "open-heart surgery" is definitely worth it, IMO.

This basically fixes the gap that the infobox added in openshift#648 warns about.
Instead of relying on the CoreOS release browser differ, we directly
query for the diff using `oc adm release info`, which recently learned
to do this, and render it at the bottom of the page. The `diff` link now
simply links to that section of the page.

This removes the VPN requirement on being able to see the RPM diff.

Only do this for the new layered node images, as well as for 4.19.
We could in theory do this all the way back to e.g. 4.12 when RHCOS
switched to "OSTree native containers", but let's be conservative. This
provide a natural rollout of the feature in case it needs ironing out.

Implementation-wise, we don't want to block the main changelog render on
getting the RPM diff. So we structure this the same way as the changelog
render itself; if the `oc` command takes too long, we display a nice
message to check again later. Once the command finishes and the output
is cached, we include it in the following render requests.
@jlebon
Copy link
Member Author

jlebon commented Feb 24, 2025

Tweaked a few things here:

  1. Dropped a bunch of unused arguments.
  2. Fixed the fact that we were always rendering the "CoreOS Package Diff" section even if the node image didn't change.
  3. More clearly gated this on 4.19 and layered node images to make it less risky.
  4. The diff is now rendered as Markdown and we convert to HTML, similarly to the main changelog page.
  5. We reprint the "CoreOS upgraded from" line, with build ID links, from the Components section in the "CoreOS Package Diff" section.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 24, 2025

@jlebon: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/govulncheck 085e671 link false /test govulncheck
ci/prow/security 085e671 link false /test security

Full PR test history. Your PR dashboard.

Details

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

@jlebon
Copy link
Member Author

jlebon commented Mar 18, 2025

OK, hopefully openshift/oc#1966 merges soon ish with the latest update there.

One thing to mention here though is that the diffUrl is still pointing at the RHCOS release browser, which will be wrong. This shows up in e.g. Sippy. We should instead link to the release controller API web UI itself for the relevant release, at the #coreos-package-diff anchor. The problem is, AFAICT, the release-controller-api has no knowledge of its own URL. Ideally we could use a relative URL, but that's also awkward because we're within the api/ namespace of the routing bits. So... not sure what the best approach is there but punted on this for now.

Maybe simplest is to allow passing release-controller-api what its base URL is so that URLs relative to that can be constructed and fed as part of the api/ interfaces? Not sure if this would be generically useful for other things than just this.

@bradmwilliams
Copy link
Collaborator

I have reviewed the code and everything seems responsible to me. Because this code is completely dependent on another PR and the promotion of the updated image, I'm going to withhold my LGTM until all the pieces are in place.

Copy link
Collaborator

@bradmwilliams bradmwilliams left a comment

Choose a reason for hiding this comment

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

/unhold

@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 7, 2025
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 7, 2025
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 7, 2025

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

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

@openshift-merge-bot openshift-merge-bot bot merged commit d9eef44 into openshift:main Apr 7, 2025
6 of 8 checks passed
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.

2 participants