build: Use rpm-ostree compose --print-only to expand ${basearch}#192
build: Use rpm-ostree compose --print-only to expand ${basearch}#192jlebon merged 1 commit intocoreos:masterfrom
Conversation
This way we can use `${basearch}` in the ref. Also the way
we were doing it before didn't work with treefile inheritance.
Reinstates support for coreos/fedora-coreos-config#24
lucab
left a comment
lucab
left a comment
There was a problem hiding this comment.
LGTM (with a general question on quoting)
| # TODO - allow this to be unset | ||
| export ref=$(manifest_get '["ref"]') | ||
| export ref=$(jq -r '.ref' < ${manifest_tmp_json}) | ||
| rm -f ${manifest_tmp_json} |
There was a problem hiding this comment.
Any reason why we don't quote variables (here and everywhere else too)?
There was a problem hiding this comment.
Yeah, consistency is not strong on this. Too bad there's not a "strict mode" for this. But probably our best bet really is to rewrite this in not-bash...
There was a problem hiding this comment.
I definitely do think about quoting whenever a variable "could reasonably" have spaces; definitely when handling externally provided filename arguments for example.
But in practice we don't support spaces for these variables like workdir, ref, name etc. So my inclination is to go with this as is - that OK with you?
There was a problem hiding this comment.
Sure, I was mostly asking for my own knowledge in case I was missing some other details, not a blocker. It makes sense to bucket this under #161.
On other coreos projects we use https://github.com/koalaman/shellcheck, which helps catching simple things.
There was a problem hiding this comment.
Ooh, wow. That is a useful project. I just ran it against cmdlib.sh and indeed...lots to fix.
| echo "Using manifest: ${manifest}" | ||
|
|
||
| manifest_tmp_json=${workdir}/tmp/manifest.json | ||
| rpm-ostree compose tree --repo=repo --print-only ${manifest} > ${manifest_tmp_json} |
There was a problem hiding this comment.
@jlebon does this extra rpm-ostree compose invocation have implications on the supermin PR?
There was a problem hiding this comment.
That should be fine; --print-only should work without any privileges at all.
There was a problem hiding this comment.
what i was referring to is the repo we operate against. Will it perform any write operations to the repo. Do we need to spin up a VM to do this in the supermin case for consistency?
There was a problem hiding this comment.
Will it perform any write operations to the repo.
If it does, that's something we would fix in rpm-ostree. :)
|
Merging this so I can rebase my work on top and make sure things still work. |
4 participants
This way we can use
${basearch}in the ref. Also the waywe were doing it before didn't work with treefile inheritance.
Reinstates support for coreos/fedora-coreos-config#24