-
Notifications
You must be signed in to change notification settings - Fork 26
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
feat: support shared components and providers #381
base: main
Are you sure you want to change the base?
Conversation
a174d66
to
b64dca9
Compare
acac964
to
4612b53
Compare
Signed-off-by: Brooks Townsend <[email protected]> fix: shared components id generation Signed-off-by: Brooks Townsend <[email protected]>
Signed-off-by: Brooks Townsend <[email protected]>
Signed-off-by: Brooks Townsend <[email protected]>
Signed-off-by: Brooks Townsend <[email protected]>
38a1ca9
to
cbb8c30
Compare
Signed-off-by: Brooks Townsend <[email protected]>
899499f
to
9ff14d5
Compare
Signed-off-by: Brooks Townsend <[email protected]>
9ff14d5
to
78702c1
Compare
// This is a problem because of our left-folding config implementation. A shared application | ||
// could specify additional config and actually overwrite the original manifest's config. | ||
(None, Some(shared_properties)) if !config.is_empty() => { | ||
failures.push(ValidationFailure::new( | ||
ValidationFailureLevel::Error, | ||
format!( | ||
"Shared component '{}' cannot specify additional 'config'", | ||
shared_properties.name | ||
), | ||
)); | ||
} |
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.
Related, would specifying secrets
here be a problem? Or perhaps not a problem, but create unnecessary work for the host when it won't actually be passed to the shared component?
I'm not sure that it necessarily makes sense to address in this PR, but something that I think needs to be addressed before this feature makes it into a release / becomes available for use is the reporting from a shared application/component's perspective. If I'm providing a shared component to the lattice, I would like to be able to query what all applications are linking against my shared component so that I can have a conversation with the teams responsible for those applications about the changes I may need to make. As I currently understand things, this change works well from the perspective of establishing a link to a shared component, but as of right now there is nothing to help the teams providing these shared components understand their "dependents", which is (imo) an absolute necessity for any platform-ish teams to be able to successfully operate. The other thing that I'm not quite clear on is how would an upgrade to/of a shared component be facilitated, would you be forced to spin up a new application (or perhaps add a new shared component to your existing application) with a different/newer version and then encourage the internal teams to upgrade? |
Feature or Problem
This PR adds support to wadm for two major changes:
wasmcloud.dev/shared: 'true'
annotation, which marks an application as shared to the entire latticeComponents and capability providers can now specify a shared application in their properties rather than supplying an image reference. When an application is deployed that references a component in a different application, wadm will validate that the application is deployed and that the referenced component exists.
You cannot use the shared component feature to alter configuration, identifiers, or scaling properties of a shared component. I could see a feature in the future to add additional scalers, but it might cause undesirable behavior and it would be generally just better to supply your own component instead of using a shared one at this point.
This PR is getting really large, so I'm going to push off additional validation in the handlers to a separate PR that can be reviewed independently.
Here are things that should be done before this feature lands, but based on their checked status aren't completed in this PR:
Related Issues
Fixes #277
Release Information
wadm 0.16
Consumer Impact
This doesn't include any breaking changes for existing manifests
Testing
Unit Test(s)
convert.rs
file that prove that the conversion between a manifest + scalers is properly handled, given how critical and complex that code is. This is made harder by the need for a bunch of genericsAcceptance or Integration
Added the
e2e_shared
integration test suite that tests sharing components, providers, and multiple aspects of linking to/from shared components.Manual Verification