-
Notifications
You must be signed in to change notification settings - Fork 168
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
plume: add cosa2stream #2000
plume: add cosa2stream #2000
Conversation
Skipping CI for Draft Pull Request. |
Draft since this is obviously incomplete, but putting up here for early feedback. |
0d159be
to
6709d74
Compare
Updated 🆕
|
6709d74
to
fc36014
Compare
In realtime chat we decided to try instead to further clean up the streams stuff in FCOS and pull the relevant cosa -> release and release -> stream bits into coreos-assembler. |
fc36014
to
9baf3b2
Compare
OK piled on more generate-release-meta improvements, and rebased |
ab31a7a
to
60910ee
Compare
OK with another fix or two, this now outputs sensible-looking data:
(We'd have work to make the |
src/cmd-generate-release-meta
Outdated
@@ -35,13 +36,9 @@ parser = ArgumentParser() | |||
parser.add_argument("--workdir", help="cosa workdir") | |||
parser.add_argument("--build-id", help="build id") | |||
parser.add_argument("--output", help="Output to file; default is build directory") | |||
parser.add_argument("url", metavar='URL', help="URL to a coreos-assembler meta.json", default=[], nargs='*') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually need this? Can't we just have the pipeline do cosa buildprep
first? That seems more in line with the rest of cosa.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's so convenient though - e.g. when working on patches to openshift/installer I just ran this command locally to generate the stream json to commit to that repo (because we haven't deployed this yet).
We don't need the ostree tarball for this either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cosa buildprep
by default doesn't fetch the OSTree tarball, so it's a fairly lightweight operation.
Hmm, I think it's the "positionalness" of the argument that seems out of place to me. WDYT about making it a repeatable --meta-json
instead?
Related: for hacking purposes, might as well have the URI support local paths to meta.json
s too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switched to --url
.
Related: for hacking purposes, might as well have the URI support local paths to meta.jsons too?
When would that happen? Feels like you either have a local cosa build or a remote URL?
60910ee
to
ca76a9a
Compare
This avoids an indent; pass in the JSON data rather than having it under a `with` statement. Prep for further work.
This is convenient.
This makes it convenient to fork this and pass it a temporary file.
Needed for RHCOS.
Prep for further work on openshift/os#477
The command currently derives the cosa branch to determine the release stream ID; for RHCOS we have `HEAD` there. Support overriding it.
Cleanup from review.
Part of implementing openshift/os#477 I don't think for RHCOS (at least initially) we will go through the whole "release.json" middle ground, so let's add a command to directly turn 1 or more cosa build(s) into a stream JSON. Because the "cosa build" -> "release" bits are in Python, we semi-hackily fork it off as a subprocess. But, it works.
ca76a9a
to
6cd3635
Compare
OK trying to port openshift-install to streams, one thing I'm hitting is that it's using the uncompressed sha256 for qemu/openstack, which isn't in streams or release metadata. Any objections to adding that? It's actually good to check both IMO. |
@cgwalters I don't see a problem adding that to the stream metadata, although I'm worried about the logic which is consuming that (which perhaps should be changed). |
Adding uncompressed SHAs seems reasonable to me too. We should update the files in https://github.com/coreos/fedora-coreos-tracker/tree/master/metadata/stream if we do this. |
To clearly show that url and local workdir can both be used.
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cgwalters, jlebon 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 |
Moving this from coreos/coreos-assembler#2000 (comment) I'm trying to port openshift-install to streams, and it currently is using the uncompressed sha256 - and further it's exposed in some cases to users. Since compression isn't always easily reproducible, I think what cosa is doing in having both checksums is a good thing; might as well do belt-and-suspenders. To emphasize we already have this data from the cosa metadata, this is just adding support for it to the stream and release metadata.
Part of implementing openshift/os#477
For now I decided to go directly from cosa to a stream because: