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: make import/export work with project patterns #4652

Merged
merged 20 commits into from
Sep 12, 2023

Conversation

thomasheartman
Copy link
Contributor

@thomasheartman thomasheartman commented Sep 11, 2023

This PR adds feature name pattern validation to the import validation step. When errors occur, they are rendered with all the offending features, the pattern to match, plus the pattern's description and example if available.

image

To achieve this I've added an extra method to the feature toggle service that checks feature names without throwing errors (because catching n async errors in a loop became tricky and hard to grasp). This method is also reused in the existing feature name validation method and handles the feature enabled chcek.

In doing so, I've also added tests to check that the pattern is applied.

@vercel
Copy link

vercel bot commented Sep 11, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
unleash-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 12, 2023 8:09am
unleash-monorepo-frontend ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 12, 2023 8:09am

Copy link
Contributor

@kwasniew kwasniew left a comment

Choose a reason for hiding this comment

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

I'd test those scenarios if you haven't done so:

  • what happens when we call import API directly (bypassing validation stage we force in the UI)
  • what happens when we use the import from the enterprise version (it's using OSS import but I'd check anyway)

@thomasheartman
Copy link
Contributor Author

@kwasniew Good suggestions; will do! I actually started on the first suggestion, but it threw some seemingly unrelated errors, but I'll revisit it. As for the second one: do you mean to add a test for this in the enterprise repo as well?

@thomasheartman
Copy link
Contributor Author

When you call the API directly, bypassing the validation, you get a 400 saying that the feature (named X) doesn't match the project's pattern Y. So I'd say that's as expected. If you supply multiple features that don't match, only the first one is returned in the error. This is probably due to how import/export works (does one at a time, exiting on error?), so I don't think changing that is within the scope of this change.

@kwasniew
Copy link
Contributor

@thomasheartman As long as we stop people from a direct import with any error message it's fine. Validation is how you get detailed errors. Direct import should just fail when you do anything illegal and it should fail with the first error.

@thomasheartman thomasheartman merged commit 9114969 into main Sep 12, 2023
16 checks passed
@thomasheartman thomasheartman deleted the 1-1346-make-importexport-work-with-patterns branch September 12, 2023 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants