-
Notifications
You must be signed in to change notification settings - Fork 109
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
ostree: add convenience function for using default OSTree deployment #1553
ostree: add convenience function for using default OSTree deployment #1553
Conversation
This obsoletes #1527 |
ok over in #1527 (comment) we came up with something like this as a proposal:
When implementing that we thought it would be more appropriate for the new option to be under
Since this was a change from what we talked about before I figured it'd be worth calling out and asking if that is an acceptable path forward. |
32a0e4d
to
5b33503
Compare
ok rebased and did a new push. Hopefully CI passes now. |
5b33503
to
a51b4ac
Compare
a51b4ac
to
ef7f48e
Compare
rebased and fixed the merge conflict! |
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.
Since the idea behind this PR was discussed in a previous one (#1527 (comment)) and I know others have thoughts on the value of being explicit, I'm going to ask for more people so sign off on this change.
@thozza @bcl @mvo5
EDIT: Consider this an approval from me.
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.
LGTM. 👍
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.
This looks fine to me. I have some suggestions inline but it's all nitpicks or ideas that are totally optional. I have no strong opinion about the feature itself. It seems fine to me to infer some related things from the tree (but I think it does technically violate some of the principles in #1568). OTOH these principles are not set in stone and I think we have other cases where things are inferred.
I attached a small patch with my suggestions and small tests as I was playing around a bit with the code but it can be a) a new PR or b) ignored as it's just tweaks.
0001-ostree-small-tweaks-for-parse_deployment_option.patch.txt
This adds a `default: true` option for all cases where OSTree information is specified in schemas and allows for the information to be picked up from the filesystem. This is a safe operation because when building disk images there is no known case where having two deployments makes sense. In the case there ever were a case then the osname, ref, and serial options still exist and can be used. Co-authored-by: Luke Yang <luyang@redhat.com> Co-authored-by: Michael Vogt <michael.vogt@gmail.com>
ef7f48e
to
cdf694e
Compare
I applied this patch and marked you as Co-Author of the commit! |
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.
Thank you!
Add the "Default" and "Source" options to OSTreeMountDeployment. See - "default" osbuild/osbuild#1553 - "source" osbuild/osbuild#1535
Add the "Default" and "Source" options to OSTreeMountDeployment. See - "default" osbuild/osbuild#1553 - "source" osbuild/osbuild#1535
Add the "Default" and "Source" options to OSTreeMountDeployment. See - "default" osbuild/osbuild#1553 - "source" osbuild/osbuild#1535
This adds a
default: true
option for all cases where OSTree information is specified in schemas and allows for the information to be picked up from the filesystem.This is a safe operation because when building disk images there is no known case where having two deployments makes sense. In the case there ever were a case then the osname, ref, and serial options still exist and can be used.