-
Notifications
You must be signed in to change notification settings - Fork 521
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
prairiedog: migrate to config file #3713
prairiedog: migrate to config file #3713
Conversation
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.
Cool! For some feedback on git style:
- I would squash
prairiedog: fix source number to match with other templates
to be included in the same commit that modified the RPM. - We tend to leave PR/fixup style comments to the PRs themselves, and the commit messages just focus on the unique change made in that commit. For consistency, it would be best to remove the
fixup!
lines.
sources/api/migration/migrations/v1.18.1/prairiedog-config-file-v0-1-0/src/main.rs
Outdated
Show resolved
Hide resolved
4aaf97d
to
9714943
Compare
squashed commits and updated changes via comments |
sources/api/migration/migrations/v1.18.1/prairiedog-config-file-v0-1-0/src/main.rs
Outdated
Show resolved
Hide resolved
sources/api/migration/migrations/v1.18.1/prairiedog-config-file-v0-1-0/src/main.rs
Outdated
Show resolved
Hide resolved
9714943
to
54edd28
Compare
Rebased off latest |
54edd28
to
5af8abe
Compare
Adjust spaces in template |
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.
Some lints are failing.
Also, we do not want to bump Release.toml and Twoliter.toml versions as part of this PR. That should be done separately then these PRs should be rebased.
Let's merge this first #3768 then rebase your PR to align on 1.19.2 |
1f8500c
to
4eab3cf
Compare
Rebased off latest |
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.
Ben's comments are blocking as well as my request that you remove the dependency on models
. For future reference, removing the dependency on models
is the reason we are doing this. If we forgot to do that on the previously merged PR, please do a follow up. Thanks!
f708642
to
fe6b525
Compare
Models dependency removed |
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.
Close, but it still has the models dependency.
fe6b525
to
8875dca
Compare
8875dca
to
170bb08
Compare
rebased off latest |
packages/os/prairiedog-toml
Outdated
{{#if settings.boot}} | ||
{{#if settings.boot.reboot-to-reconcile}} | ||
reboot-to-reconcile = {{settings.boot.reboot-to-reconcile}} | ||
|
||
{{/if}} | ||
{{#if settings.boot.kernel}} | ||
[kernel] | ||
{{#each settings.boot.kernel}} | ||
"{{@key}}" = [ {{#each this}}"{{{this}}}",{{/each}} ] | ||
{{/each}} | ||
|
||
{{/if}} |
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.
Does this render a config file with two empty lines if none of the settings are present?
Changes prairiedog to read from /etc/prairiedog.toml which is generated automatically from the settings.
170bb08
to
efa092b
Compare
Issue number: #3622
Closes #3622
Description of changes:
Testing done:
Fully tested migration upgrade and the reboot logic for the reconcile works. Also made sure kernel settings synced to the file
Terms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.