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

prairiedog: migrate to config file #3713

Merged
merged 1 commit into from
Feb 28, 2024

Conversation

jmt-lab
Copy link
Contributor

@jmt-lab jmt-lab commented Jan 17, 2024

Issue number: #3622

Closes #3622

Description of changes:

  • Migrates prairiedog from fetching BootSettings from settings api to fetching this from /etc/prairiedog.toml or config file provided by -c/--config-path
  • Adds template prairiedog-toml and migration

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.

Copy link
Contributor

@cbgbt cbgbt left a 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/prairiedog/src/bootconfig.rs Show resolved Hide resolved
packages/os/prairiedog-toml Outdated Show resolved Hide resolved
@jmt-lab
Copy link
Contributor Author

jmt-lab commented Jan 18, 2024

squashed commits and updated changes via comments

packages/os/prairiedog-toml Outdated Show resolved Hide resolved
packages/os/prairiedog-toml Outdated Show resolved Hide resolved
sources/api/prairiedog/src/main.rs Outdated Show resolved Hide resolved
sources/api/prairiedog/src/bootconfig.rs Show resolved Hide resolved
@jmt-lab
Copy link
Contributor Author

jmt-lab commented Feb 6, 2024

Rebased off latest
Split migrations

@jmt-lab
Copy link
Contributor Author

jmt-lab commented Feb 6, 2024

Adjust spaces in template

Copy link
Member

@webern webern left a 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.

@webern
Copy link
Member

webern commented Feb 7, 2024

Let's merge this first #3768 then rebase your PR to align on 1.19.2

@jmt-lab jmt-lab force-pushed the jmt/prairiedog/config-file branch 2 times, most recently from 1f8500c to 4eab3cf Compare February 13, 2024 21:34
@jmt-lab
Copy link
Contributor Author

jmt-lab commented Feb 13, 2024

Rebased off latest
Fixed nested quotes in template
Fixed formatting

Copy link
Member

@webern webern left a 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!

sources/api/prairiedog/Cargo.toml Show resolved Hide resolved
@jmt-lab jmt-lab force-pushed the jmt/prairiedog/config-file branch 5 times, most recently from f708642 to fe6b525 Compare February 19, 2024 20:52
@jmt-lab
Copy link
Contributor Author

jmt-lab commented Feb 19, 2024

Models dependency removed

@jmt-lab jmt-lab requested a review from webern February 19, 2024 21:37
Copy link
Member

@webern webern left a 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.

sources/api/prairiedog/Cargo.toml Outdated Show resolved Hide resolved
@jmt-lab
Copy link
Contributor Author

jmt-lab commented Feb 28, 2024

rebased off latest

Comment on lines 4 to 13
{{#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}}
Copy link
Contributor

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.
@jmt-lab jmt-lab merged commit f6c1b7a into bottlerocket-os:develop Feb 28, 2024
50 checks passed
@bcressey bcressey changed the title Jmt/prairiedog/config file prairiedog: migrate to config file May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OOTB: Remove model dependency from prairiedog
4 participants