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

Fix: Validate project_role #143

Merged
merged 1 commit into from
Sep 5, 2021
Merged

Conversation

liranfarage89
Copy link
Contributor

Issue & Steps to Reproduce / Feature Request

While QAing #140 , @RLRabinowitz notice that on PR-Plan we don't validate the role.

Solution

validate it.

@liranfarage89 liranfarage89 changed the title qa-comments fix validate-func of project_role Fix: Validate project_role Sep 5, 2021
@liranfarage89 liranfarage89 self-assigned this Sep 5, 2021
Copy link
Contributor

@avnerenv0 avnerenv0 left a comment

Choose a reason for hiding this comment

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

TBH not loving this implementation,
Role should probably be a stronger type than string and also it's not the responsibility of resource_team_project_assignment to validate Role values, that should be in the model or similar.

However - not critical AT ALL so definitely approved 👍

@liranfarage89 liranfarage89 merged commit 69b8204 into main Sep 5, 2021
@liranfarage89 liranfarage89 deleted the fix-project-role-validation branch September 5, 2021 14:13
@liranfarage89
Copy link
Contributor Author

@avnerenv0
I've started something(very quickly), but the tests need to be changed, which is the most of work here.

how would you suggest handle that? opening a PR with "doing on the side"?

https://github.com/env0/terraform-provider-env0/tree/chore-move-role-validation (see the last commit)

@avnerenv0
Copy link
Contributor

@liranfarage89

opening a PR with "doing on the side"?
Nah, if you wanna do it just go ahead and get it over with, not "on the side".
Just let our dev channel know that you want to complete this part, and keep an eye on the ROI of the task, if it get's messy probably leave it.

BTW we talked about breaking that model file into file-per-model, you might as well put Role in it's own file.

@liranfarage89
Copy link
Contributor Author

liranfarage89 commented Sep 5, 2021

@avnerenv0
that's ROI negative currently, that's why I thought about "doing on the side" 😕

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants