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

Validate variable and config key names at app load #1623

Merged
merged 1 commit into from
Jun 29, 2023

Conversation

itowlson
Copy link
Contributor

This fixes #1622 as far as we can go within Spin. I think that the cloud plugin takes the manifest before validation (the local load pipeline doesn't come apart at the right place) so this would need to be ported to the cloud plugin separately.

Signed-off-by: itowlson <ivan.towlson@fermyon.com>
@itowlson
Copy link
Contributor Author

Looking deeper at this, the added dependency on spin-config drags spin-app and spin-core as indirect dependencies of spin-loader, which is a lot of weight to add to any plugin that just wants to read a dashed manifest. So maybe it would be better to duplicate the validation criteria into spin-loader. Or we could wham a Cargo feature into spin-config. @lann do you have opinions?

@fibonacci1729
Copy link
Contributor

While discussing this earlier, @lann and I had envisioned a check crate that could be shared between spin, lhc-prepare, and the cloud-plugin. This feels more sippable as we'd likely want to gather consensus on what is in the realm of doctor and this validation step (or atleast provide guidance) so perhaps not something to try to address in this PR.

This is my fault though I should have updated the issue with some more info re: sharing this validation logic with lhc so my apologies Ivan!

I think it makes sense in the interim to implement something like what you are proposing here though because I hit this issue earlier on when building a demo app for config (as I forgot to consult the dynaconfig docs)

@itowlson
Copy link
Contributor Author

@fibonacci1729 Yeah, the cloud plugin at least currently depends on the runtime through other avenues (see #1608) so it's not a huge deal for that. I'm happy to let this stand and re-assess in a future PR.

Thanks for sharing your discussion - I agree it would be good to figure out more broadly what might be useful to alternate tools and hosts and how to surface it. E.g. a check crate could be useful but keeping validation logic with the validatees is also useful; what about things like the info endpoint schema; etc. But the feature route risks being more complicated to manage. So 🤷

@itowlson
Copy link
Contributor Author

Would somebody be willing to either approve or ask for changes please? Thanks!

Copy link
Contributor

@fibonacci1729 fibonacci1729 left a comment

Choose a reason for hiding this comment

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

LGTM! This is useful today even if we decide to factor things differently in the near future. Thanks @itowlson !

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.

Detect malformed configuration variable keys before up/deploy
3 participants