Skip to content

ostree: add convenience function for using default OSTree deployment#1553

Merged
dustymabe merged 1 commit intoosbuild:mainfrom
dustymabe:dusty-default-ostree-deployment
Feb 7, 2024
Merged

ostree: add convenience function for using default OSTree deployment#1553
dustymabe merged 1 commit intoosbuild:mainfrom
dustymabe:dusty-default-ostree-deployment

Conversation

@dustymabe
Copy link
Contributor

@dustymabe dustymabe commented Jan 25, 2024

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.

@dustymabe
Copy link
Contributor Author

dustymabe commented Jan 25, 2024

This obsoletes #1527

This was referenced Jan 25, 2024
Closed
Closed
cgwalters
cgwalters reviewed Jan 25, 2024
View reviewed changes
@dustymabe
Copy link
Contributor Author

dustymabe commented Jan 25, 2024

ok over in #1527 (comment) we came up with something like this as a proposal:

So the concensus is something like OneOf:

"deployment": {
  "osname": ...,
  "ref": ...
  "serial": ... (default 0)
}

or

"default-deployment": true (default false)

When implementing that we thought it would be more appropriate for the new option to be under deployment and not a new top level property. So that is what we came up with here where we now allow the user to specify something like:

"deployment": {
  "default": true
}

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.

dustymabe
dustymabe commented Jan 25, 2024
View reviewed changes
@dustymabe dustymabe force-pushed the dusty-default-ostree-deployment branch from 32a0e4d to 5b33503 Compare January 26, 2024 18:35
@dustymabe
Copy link
Contributor Author

dustymabe commented Jan 26, 2024

ok rebased and did a new push. Hopefully CI passes now.

@dustymabe dustymabe mentioned this pull request Jan 26, 2024
Closed
@dustymabe dustymabe force-pushed the dusty-default-ostree-deployment branch from 5b33503 to a51b4ac Compare January 26, 2024 19:41
@dustymabe dustymabe force-pushed the dusty-default-ostree-deployment branch from a51b4ac to ef7f48e Compare February 6, 2024 16:06
@dustymabe
Copy link
Contributor Author

dustymabe commented Feb 6, 2024

rebased and fixed the merge conflict!

achilleas-k
achilleas-k reviewed Feb 6, 2024
View reviewed changes
Copy link
Member

@achilleas-k achilleas-k left a comment •
edited
Loading

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.

thozza
thozza previously approved these changes Feb 7, 2024
View reviewed changes
Copy link
Member

@thozza thozza left a comment

Choose a reason for hiding this comment

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

LGTM. 👍

@thozza thozza requested a review from mvo5 February 7, 2024 10:27
mvo5
mvo5 previously approved these changes Feb 7, 2024
View reviewed changes
Copy link
Contributor

@mvo5 mvo5 left a 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

@dustymabe @lukewarmtemp @mvo5
ostree: add convenience function for using default OSTree deployment
cdf694e 
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>
@dustymabe dustymabe dismissed stale reviews from mvo5 and thozza via cdf694e February 7, 2024 14:52
@dustymabe dustymabe force-pushed the dusty-default-ostree-deployment branch from ef7f48e to cdf694e Compare February 7, 2024 14:52
@dustymabe
Copy link
Contributor Author

dustymabe commented Feb 7, 2024

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

I applied this patch and marked you as Co-Author of the commit!

@dustymabe dustymabe enabled auto-merge (rebase) February 7, 2024 14:52
mvo5
mvo5 approved these changes Feb 7, 2024
View reviewed changes
Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

Thank you!

@dustymabe dustymabe merged commit e1cbf92 into osbuild:main Feb 7, 2024
dustymabe added a commit to dustymabe/coreos-assembler that referenced this pull request Feb 10, 2024
@dustymabe
osbuild: backport optimization for using OSTree default deployment
8908532 
Upstream PR: osbuild/osbuild#1553
dustymabe added a commit to coreos/coreos-assembler that referenced this pull request Feb 12, 2024
@dustymabe
osbuild: backport optimization for using OSTree default deployment
f773c8c 
Upstream PR: osbuild/osbuild#1553
@mvo5 mvo5 mentioned this pull request Mar 26, 2024
Closed
mvo5 added a commit to mvo5/images that referenced this pull request Apr 5, 2024
@mvo5
osbuild: add new osbuild.NewOSTreeDeploymentMountDefault()
db25aaa 
Add the "Default" and "Source" options to OSTreeMountDeployment.
See
- "default" osbuild/osbuild#1553
- "source"  osbuild/osbuild#1535
mvo5 added a commit to mvo5/images that referenced this pull request Apr 5, 2024
@mvo5
osbuild: add new osbuild.NewOSTreeDeploymentMountDefault()
17476de 
Add the "Default" and "Source" options to OSTreeMountDeployment.
See
- "default" osbuild/osbuild#1553
- "source"  osbuild/osbuild#1535
@mvo5 mvo5 mentioned this pull request Apr 5, 2024
Merged
github-merge-queue bot pushed a commit to osbuild/images that referenced this pull request Apr 7, 2024
@mvo5 @ondrejbudai
osbuild: add new osbuild.NewOSTreeDeploymentMountDefault()
3039ddb 
Add the "Default" and "Source" options to OSTreeMountDeployment.
See
- "default" osbuild/osbuild#1553
- "source"  osbuild/osbuild#1535
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cgwalters cgwalters cgwalters left review comments

@achilleas-k achilleas-k achilleas-k left review comments

@thozza thozza thozza left review comments

+1 more reviewer

@mvo5 mvo5 mvo5 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@dustymabe @cgwalters @mvo5 @achilleas-k @thozza