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

Fix missing extensions in RHCOS release browser #4009

Merged
merged 3 commits into from
Jan 31, 2025
Merged

Conversation

ravanelli
Copy link
Member

No description provided.

@ravanelli
Copy link
Member Author

It depends on openshift/os#1719

@@ -129,9 +129,6 @@ type Cloudartifact struct {

type Extensions struct {
Manifest map[string]interface{} `json:"manifest"`
Path string `json:"path"`
RpmOstreeState string `json:"rpm-ostree-state"`
Sha256 string `json:"sha256"`
Copy link
Member

Choose a reason for hiding this comment

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

I agree that I don't think we need these any longer. My only concern is do we somehow want to maintain backward compatibility in case we encounter this (i.e. using current tools to look at an older build).

Copy link
Member Author

Choose a reason for hiding this comment

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

We would have already noticed this since no extensions were being created at all. I suggest backporting until the point where we stopped generating extensions in meta.json.

Copy link
Member

@dustymabe dustymabe Jan 29, 2025

Choose a reason for hiding this comment

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

Not sure I understand.. but let me ask the question a different way..

If you do bake this PR into a COSA image and then try to parse (i.e. buildfetch and cosa meta --build=415.92.202501281917-0 --get extensions) does it barf on 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 important to use 4.15 in this test because those builds do have the extensions in the meta.json

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it does work:

cosa meta --get extensions
warning: /sys/fs/selinux appears to be mounted but should not be; enabling workaround
extensions: {'path': 'rhcos-415.92.202501281917-0-extensions.x86_64.tar', 'sha256': '32166c5e89e461d2b5fe1487281e4d5c76369ad51cef5a7252de30b0cad9da9d',
'rpm-ostree-state': '50ac76b51fccfe9cd628f009e3513747c3c9c0ca645315dfea249fc37b2a176e4874b84afe437d86a3e964d25cec91d764af007164b4424b603b7fcd2718009f', 
'manifest': {'NetworkManager-libreswan': '1.2.14-6.el9_2.x86_64',
...

The only diff for now is that we won't have path, sha256 and rpm-ostree-state, but does work in the same way.

Copy link
Member

Choose a reason for hiding this comment

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

right. I was just worried that the schema would error if there were components in there that it didn't expect.

Copy link
Member

Choose a reason for hiding this comment

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

It works because the schema doesn't disallow additional properties. So the fields are essentially "unregistered". I think instead of this, we should make those properties optional. That way we still have type modelling for them. This could be useful in the future for GC.

@ravanelli ravanelli force-pushed the pr/extensions branch 4 times, most recently from 094d478 to c778b0f Compare January 29, 2025 18:59
dustymabe
dustymabe previously approved these changes Jan 29, 2025
@ravanelli
Copy link
Member Author

/retest

@ravanelli
Copy link
Member Author

ravanelli commented Jan 30, 2025

The probably need openshift/os#1719 to get merged first to make prow happy

@ravanelli ravanelli requested review from jlebon and dustymabe January 30, 2025 14:52
Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

Looks sane overall!

@@ -129,9 +129,6 @@ type Cloudartifact struct {

type Extensions struct {
Manifest map[string]interface{} `json:"manifest"`
Path string `json:"path"`
RpmOstreeState string `json:"rpm-ostree-state"`
Sha256 string `json:"sha256"`
Copy link
Member

Choose a reason for hiding this comment

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

It works because the schema doesn't disallow additional properties. So the fields are essentially "unregistered". I think instead of this, we should make those properties optional. That way we still have type modelling for them. This could be useful in the future for GC.

@ravanelli ravanelli force-pushed the pr/extensions branch 2 times, most recently from 4d54b85 to 4028795 Compare January 30, 2025 19:30
- Make `path`, `sha256` and `rpm-ostree-state` optional fields in `meta.json`
for extensions, as no tar file was ever stored as an artifact.
Including references to a non-existent file is unnecessary.

Signed-off-by: Renata Ravanelli <[email protected]>
- Fix an issue where extensions were not displayed in
the RHCOS release browser for versions 4.16 and later.

Signed-off-by: Renata Ravanelli <[email protected]>
- The repository https://github.com/idubinskiy/schematyper has not seen
any commits in 7 years and appears to be unmaintained. Since the package
`alecthomas/kingpin` name has changed, fix it and build it locally. This
avoids relying on `go install` and allows us to make necessary
fixes to the package as needed.

Signede off-by: Renata Ravanelli <[email protected]>
Copy link
Member

@jlebon jlebon 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

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

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

/lgtm

@ravanelli
Copy link
Member Author

/retest

@ravanelli ravanelli merged commit d87064a into main Jan 31, 2025
5 checks passed
@ravanelli ravanelli deleted the pr/extensions branch January 31, 2025 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants