-
Notifications
You must be signed in to change notification settings - Fork 492
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
Jmt/ootb/pluto #3828
Jmt/ootb/pluto #3828
Conversation
Fix formatting |
Rebased, build fixes, tests ran |
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.
You need a clippy fix
|
7c37bba
to
28e0a72
Compare
Why? Generally speaking the whimsical names shouldn't intrude into the visible surface area of the distro. For OOTBs that will also include package names. Historical aside: Back on topic: I am more OK with the generic ones like Aside from not wanting to export whimsy or to explain |
I was just following netdog since we pulled it out when doing a similar conditional removement, I'm okay sliding this back into os.spec if that is preferred |
53f936c
to
0bb2977
Compare
Updated to ensure the compilation changes with pluto & cfsignal work well. Note I will be addressing shibaken and other remaining if blocks in another PR |
sources/api/migration/migration-helpers/src/common_migrations.rs
Outdated
Show resolved
Hide resolved
6a5fe0e
to
3f3f9cf
Compare
|
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.
Is the empty file packages/os/pluto-toml
intentional? If so can you explain why you need an empty file?
sources/api/migration/migration-helpers/src/common_migrations.rs
Outdated
Show resolved
Hide resolved
Removed unnecessary test |
Interactive rebase done to better arrange changes |
f929d97
to
2ffebfc
Compare
Rebased to squash commits and separate migration |
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.
- The Pluto code looks correct to me, to the best of my ability with the caveat that I can't quite tell if there could be a subtle behavior change based on the fact that Pluto used to be called at different times for these things and may have had different settings available when that occurred.
- The migration seems probably correct, though I have a question about "nothing to do on downgrade".
- I mostly deferring to Ben on packaging concerns.
|
||
fn backward(&mut self, input: MigrationData) -> Result<MigrationData> { | ||
println!( | ||
"RemoveMetadataMigration({:?}) has no work to do on downgrade.", |
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.
Double-checking this is right, @bcressey. We don't need to do anything on downgrade because the defaults will cause the metadata to be put back into place on downgrade, right?
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.
Double-checking this is right, @bcressey. We don't need to do anything on downgrade because the defaults will cause the metadata to be put back into place on downgrade, right?
Yes. migrator.service
runs before storewolf.service
and (simplifying) is responsible for fixing things up so the datastore that storewolf
deserializes matches what the model expects.
storewolf
will add any new defaults to an existing data store (since 036be78) but does not touch existing data.
One conclusion I've drawn from this - frankly downright weird! - migration is that there's probably something much smarter we could do for the datastore files that cannot be set via the API, such as "remove all existing metadata so that storewolf
puts it back".
But that would fall outside the scope of what we can cover in any one set of up/down migrations; we'd need the OS itself to be smart enough to do that, which means doing the repo pivot trick to move to the new world where we can treat it as a given.
If we did that, we could expand the scope to include configuration-files and services too. Might as well throw in unrecognized prefixes and individual files while we're at it, and the ability to set a field to {{ default }}
to force it to be re-evaluated to its current default value. Potentially we could eliminate almost all of what we use migrations for even in the current settings system.
|
Migrates pluto to fetch a view of required settings from the api using apiclient to generated needed settings for k*s on aws
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.
🪐
Issue number: #3620
Closes #3620
Description of changes:
Removes conditional compilation from pluto.
Testing done:
cargo +nightly udeps
to ensure all unused dependencies have been removedTested with downgrade -> upgrade
Initial Build ID 1.20.0 0bb2977
Downgrade Build ID 1.19.5 64049ba
Upgrade Build ID 1.20.0 0bb2977
Verified that pluto in both situations generated correct settings
Verified that the new remove metadata does a noop on downgrade and setting-generators return
Verified that the new remove metadata removes setting-generator settings on upgrade
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.