Skip to content

USHIFT-247: Generate release-info#1196

Merged
openshift-merge-robot merged 2 commits intoopenshift:mainfrom
fzdarsky:generate-release-info
Dec 16, 2022
Merged

USHIFT-247: Generate release-info#1196
openshift-merge-robot merged 2 commits intoopenshift:mainfrom
fzdarsky:generate-release-info

Conversation

@fzdarsky
Copy link
Copy Markdown
Contributor

@fzdarsky fzdarsky commented Dec 15, 2022

Adds a new RPM package "microshift-release-info" that contains JSON files containing the container image digests of the respective MicroShift release, so users can use these digests when creating osbuild-composer blueprints.

As side-effect, moves MicroShift's own release info from the pkg/release/release_*.go into JSON files in the assets/release directory.

Signed-off-by: Frank A. Zdarsky fzdarsky@redhat.com

Closes USHIFT-247

@openshift-ci openshift-ci Bot requested a review from zshi-redhat December 15, 2022 12:23
@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 15, 2022
@ggiguash
Copy link
Copy Markdown
Contributor

@fzdarsky, can you add a short doc for devs explaing how this information can be used out of the new RPM?

Comment thread docs/rhel4edge_iso.md Outdated
Comment thread pkg/release/release.go Outdated
Comment thread pkg/release/release.go Outdated
Comment thread scripts/image-builder/build.sh Outdated
@ggiguash
Copy link
Copy Markdown
Contributor

@fzdarsky, please update this doc section with the new RPM

Comment thread packaging/rpm/microshift.spec Outdated
@ggiguash
Copy link
Copy Markdown
Contributor

/assign ggiguash

@fzdarsky
Copy link
Copy Markdown
Contributor Author

@fzdarsky, please update this doc section with the new RPM

Done.

Comment thread pkg/release/release.go Outdated
Copy link
Copy Markdown
Contributor

@oglok oglok Dec 15, 2022

Choose a reason for hiding this comment

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

I wonder why arch names have been replaced: amd64 to x86_64 and arm64 to aarch64. Any specific reason?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

My motivation for this was user experience. The x86_64/aarch64 architecture names are more idiomatic on Linux systems in general and RPM-based distros in particular.

For example, this way users can use $(uname -i) in scripts and avoid conditional logic and substitutions as with the Golang names. It’s also consistent with the RPM package naming scheme.

@oglok
Copy link
Copy Markdown
Contributor

oglok commented Dec 15, 2022

I think it's a very smart approach to solve this problem.

Copy link
Copy Markdown
Contributor

@dhellmann dhellmann left a comment

Choose a reason for hiding this comment

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

This is pretty much exactly what I had in mind. I had one or two nits, but nothing to hold it up. I know there were some other commenters, so I'll leave a hold along with the approval and you can remove that when everyone else is satisfied.

/lgtm
/hold

Comment thread scripts/image-builder/build.sh Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We could move the base info out eventually, too. We shouldn't hold this up over that change, though.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree. I think removing get.sh script forced us to duplicate and spread parsing of this information in other scripts. Let's return the get.sh file

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Moved the base into the JSON, too, so now parsing is uniform and simple.

Comment thread packaging/rpm/microshift.spec Outdated
@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 Dec 15, 2022
@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Dec 15, 2022
@ggiguash
Copy link
Copy Markdown
Contributor

@fzdarsky, can we please implement 2 changes before we merge this PR?

  • Add a document in GitHub (referenced from the README.md) explaining the RPM usage scenarios
  • Restore the get.sh script and fix it to support the new parsing.

Comment thread pkg/release/release.go Outdated
Copy link
Copy Markdown
Contributor

@mangelajo mangelajo Dec 16, 2022

Choose a reason for hiding this comment

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

It would be great if we could also produce a toml snippet so customers just can append that to their blueprints without additional processing if they wanted.

I suggest that as follow up, not now.

The toml is less descriptive for the name of the images, but we could put that in comments.

Comment thread packaging/rpm/microshift.spec Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

May be we should explain the purpose of this; i.e.

This package can be used to find the images consumed by MicroShift in runtime
and embed those into blueprints/ostree images.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added.

@fzdarsky fzdarsky force-pushed the generate-release-info branch from b8c101d to 85c9b94 Compare December 16, 2022 12:20
@openshift-ci openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 16, 2022
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 16, 2022
@fzdarsky fzdarsky force-pushed the generate-release-info branch from 85c9b94 to 7db55f6 Compare December 16, 2022 12:32
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 16, 2022
@fzdarsky
Copy link
Copy Markdown
Contributor Author

@fzdarsky, can you add a short doc for devs explaing how this information can be used out of the new RPM?

Added new docs/howto_offline_containers.md and linked it.

@fzdarsky fzdarsky force-pushed the generate-release-info branch from 7db55f6 to f1cdc37 Compare December 16, 2022 14:23
@fzdarsky
Copy link
Copy Markdown
Contributor Author

/hold cancel

@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 Dec 16, 2022
Introduces a new subpackage microshift-release-info that contains
JSON files with the pull specs of the container images used by
the respective MicroShift version.

Also moves this release info from code (release_*.go) into a
JSON in the assets folder (release-*.json) for more robust
processing using jq. That also allows to remove the get.sh
script. Updates scripts and docs accordingly.

Signed-off-by: Frank A. Zdarsky <fzdarsky@redhat.com>
Signed-off-by: Frank A. Zdarsky <fzdarsky@redhat.com>
@fzdarsky fzdarsky force-pushed the generate-release-info branch from f1cdc37 to 8fc3c1d Compare December 16, 2022 14:31
@mangelajo
Copy link
Copy Markdown
Contributor

/lgtm

Still get.sh is not re-added as talked in slack. But we can add that back if necessary.

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Dec 16, 2022
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Dec 16, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhellmann, fzdarsky, mangelajo

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 [dhellmann,fzdarsky,mangelajo]

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

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Dec 16, 2022

@fzdarsky: all tests passed!

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

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.

6 participants