Skip to content
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

host: Switch to inline postprocess element #316

Merged

Conversation

cgwalters
Copy link
Member

The primary reason I'm doing this is that today rpm-ostree's
treefile inheritance doesn't quite do what one wants with external
files. There's some major work on that in
coreos/rpm-ostree#1574
But let's not block on/require it.

Furthermore, using the inline postprocess value allows us
to clearly demarcate the maipo-specific hacks in that file.

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 26, 2018
Information: https://url.corp.redhat.com/redhat-coreos
Bugs: https://github.com/openshift/os
---
EOF
Copy link
Member

Choose a reason for hiding this comment

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

in the old file there was an extra newline between the --- and the EOF. Don't know if we want to keep that or not or if it's possible with inline yaml

Copy link
Member Author

Choose a reason for hiding this comment

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

Now with 🆕 newline ⬇️

Copy link
Member

Choose a reason for hiding this comment

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

This is slight bikeshed but... why not keep the newline separators in the actual script itself too? Makes it much nicer to read through it. At least PyYAML definitely supports that. I'd be surprised if serde didn't.

@dustymabe
Copy link
Member

did a vimdiff between the old file and new ones. one small comment. LGTM

The primary reason I'm doing this is that today rpm-ostree's
treefile inheritance doesn't quite do what one wants with external
files.  There's some major work on that in
coreos/rpm-ostree#1574
But let's not block on/require it.

Furthermore, using the inline `postprocess` value allows us
to clearly demarcate the `maipo`-specific hacks in that file.
@cgwalters cgwalters force-pushed the postprocess-no-script branch from e5be9a1 to 2fe5800 Compare September 26, 2018 15:27
@ashcrow
Copy link
Member

ashcrow commented Sep 26, 2018

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 26, 2018
@openshift-merge-robot openshift-merge-robot merged commit 685ca4d into openshift:master Sep 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants