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

feat(validation): validation fails if component has source_config #337

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

markkovari
Copy link

Feature or Problem

Related Issues

#288

Release Information

Consumer Impact

Testing

Unit Test(s)

unit tests should cover the negative case, other validation calls don't have this error

Acceptance or Integration

Manual Verification

… definition

Signed-off-by: Márk Kővári <kovarimarkofficial@gmail.com>
@markkovari
Copy link
Author

markkovari commented Jul 17, 2024

I do have 2 questions tho:

  • is it even a valid configuration, if not feel free to point out edge cases to add
  • the validation looks similar to a callback hell, hopefully you have wide enough screens, please call me out if it needs refactor (update: definitely needs to be refactored, I forgot how to rust 😿 )

Appreciate

Signed-off-by: Márk Kővári <kovarimarkofficial@gmail.com>
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.

Hey @markkovari ! I think the structure of this PR looks great, you may just need to consider a change or two after #307. If we want to try and move this forward now, we should at least just change the parsing of the source_config key to be in a link trait

Comment on lines +15 to +23
traits:
- type: spreadscaler
properties:
instances: 1
# This is not allowed since 1.0.0 should error during validation
source_config:
- name: any_value
properties:
SOME_OTHER_KEY: any_other_value
Copy link
Member

Choose a reason for hiding this comment

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

This trait should be a link trait, just because that's where the source_config value lives

Comment on lines +420 to +421
if let TraitProperty::Custom(custom) = trait_.properties.clone() {
if let Some(_) = custom.get(forbidden_config_key) {
Copy link
Member

Choose a reason for hiding this comment

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

So just a heads up, you may have to do some reworking of this after https://github.com/wasmCloud/wadm/pull/307/files merges because the trait properties with source_config should actually parse as a link trait.

Copy link
Author

Choose a reason for hiding this comment

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

Cool, thanks for the heads up, we can definitely wait a bit with this than, I was just getting familiar with the project. 🙇

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.

None yet

2 participants