-
Notifications
You must be signed in to change notification settings - Fork 506
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
sources: move settings-sdk to workspace dependencies #4099
sources: move settings-sdk to workspace dependencies #4099
Conversation
sources/models/Cargo.toml
Outdated
# settings plugins | ||
bottlerocket-settings-models = { workspace = true } | ||
bottlerocket-settings-plugin = { workspace = true } |
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.
I wonder if this would be clearer if it remained at the bottom instead of being alphabetical.
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.
I like workspace dependencies! I would love to use them for everything under /sources
, but we don't have to do that all in one go.
Something I saw recently that I quite liked was that you can also just specify them as such:
bottlerocket-settings-models.workspace = true
...and you would then only need to expand the object if you wished to add other attributes.
What do you think?
Yeah, I think I like that too! I'll update it. |
4edb61d
to
e3e27da
Compare
^ moved settings plugins deps back to their own section and updated formatting to |
[build-dependencies.bottlerocket-defaults-helper] | ||
git = "https://github.com/bottlerocket-os/bottlerocket-settings-sdk" | ||
tag = "bottlerocket-defaults-helper-v0.1.0" | ||
version = "0.1.0" | ||
workspace = true |
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.
nit: these don't follow the format you've changed the rest to.
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.
whoops, fixed
Within the sources/ workspace, if a crate depends on one of the crates in bottlerocket-settings-sdk, we currently include a section with the full git URL, tag, and version in the crate's Cargo.toml. Because we can't have two versions of the same crate in a workspace, updating one of these Cargo.toml's (in order to test changes in a personal fork, or to bump the version, for example) requires updating all of the other crates that use that dependency. Making the crates in bottlerocket-settings-sdk workspace dependencies allows us to update to a different version of bottlerocket-settings-sdk by only changing one file. Signed-off-by: Sam Berning <bernings@amazon.com>
e3e27da
to
303b74b
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.
LGTM
Issue number:
N/A
Description of changes:
Within the sources/ workspace, if a crate depends on one of the crates in bottlerocket-settings-sdk, we currently include a section with the full git URL, tag, and version in the crate's Cargo.toml. Because we can't have two versions of the same crate in a workspace, updating one of these Cargo.toml's (in order to test changes in a personal fork, or to bump the version, for example) requires updating all of the other crates that use that dependency.
Making the crates in bottlerocket-settings-sdk workspace dependencies allows us to update to a different version of bottlerocket-settings-sdk by only changing one file.
Testing done:
cargo build
insources/
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.