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

Only check configs for unique names if they declare properties #585

Merged

Conversation

StuartHarris
Copy link
Contributor

Feature or Problem

#516 rejects manifests where multiple link configurations have the same name. This PR augments that functionality by only considering link configurations that declare properties. This is because (as currently implemented) configurations that don't declare properties can reference existing configs that are created either by wash or by wadm (prefixed by the application name, e.g. where application = "my-app" and config = "my-config", this can be referenced elsewhere in the manifest as "my_app-my_config").

Related Issues

#516
#478
#401

Release Information

Manifests that were previously rejected may now be valid. As this widens rather than narrows, I suspect it can be released as a non-breaking minor/patch release.

Consumer Impact

All manifests that were previously valid should remain valid.

Testing

Unit Test(s)

Acceptance or Integration

The existing test case for validation of duplicate config names has been adjusted to include properties for the duplicate configs. Additionally, duplicate configs without properties were added to show that they are now allowed through the validation stage.

Manual Verification

None

@StuartHarris StuartHarris requested a review from a team as a code owner February 7, 2025 10:55
@StuartHarris StuartHarris force-pushed the fix/validate-config-names branch from 89ddeb7 to 6aa6c7c Compare February 7, 2025 10:59
@StuartHarris StuartHarris force-pushed the fix/validate-config-names branch from 6aa6c7c to 9e2630f Compare February 7, 2025 11:03
Copy link
Member

@brooksmtownsend brooksmtownsend left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonderful work! LGTM 😄

@brooksmtownsend brooksmtownsend enabled auto-merge (rebase) February 7, 2025 14:07
@brooksmtownsend brooksmtownsend merged commit bdf06dc into wasmCloud:main Feb 7, 2025
5 checks passed
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.

2 participants