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.

Sign up for GitHub

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

Jump to bottom

fix(okta_auth_backend_group): allow '/' in group_name #687

Merged
merged 4 commits into from
Mar 6, 2020
Merged

fix(okta_auth_backend_group): allow '/' in group_name #687

merged 4 commits into from
Mar 6, 2020

Conversation

rasta-rocket
Copy link
Contributor

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Maybe I missed some context but why do we not allowed '/' initially in the group name ?

Release note for CHANGELOG:

Allow '/' character in the group_name field of the okta_auth_backend_group resource

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestOkta'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestOkta -timeout 120m
?       github.com/terraform-providers/terraform-provider-vault [no test files]
?       github.com/terraform-providers/terraform-provider-vault/cmd/coverage    [no test files]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-vault/util    (cached) [no tests to run]
=== RUN   TestOktaAuthBackendGroup
--- PASS: TestOktaAuthBackendGroup (4.49s)
=== RUN   TestOktaAuthBackend
--- PASS: TestOktaAuthBackend (3.99s)
=== RUN   TestOktaAuthBackendUser
--- PASS: TestOktaAuthBackendUser (8.53s)
PASS
ok      github.com/terraform-providers/terraform-provider-vault/vault   17.053s

...

@ghost ghost added the size/XS label Feb 25, 2020
@tyrannosaurus-becks tyrannosaurus-becks self-assigned this Mar 3, 2020
@tyrannosaurus-becks
Copy link
Contributor

Hi! Thanks for working on this!

My guess is that the main reason we don't allow "/" in various path-related parameters is because at times, the code needs to find out which path a particular mount of a back end is at. It's easier to figure out when you don't need to worry about extra slashes.

In our contribution guidelines, we ask for test coverage before we review PR's. Would you be willing to add tests covering all fields changed where a "/" is now allowable, and make sure they still come out with the correct mount paths? Also, the docs may need to be updated (I'm not sure).

Happy to circle back and give a review when that's done. Thanks again!

@rasta-rocket
Copy link
Contributor Author

Hi! Thanks for working on this!

My guess is that the main reason we don't allow "/" in various path-related parameters is because at times, the code needs to find out which path a particular mount of a back end is at. It's easier to figure out when you don't need to worry about extra slashes.

In our contribution guidelines, we ask for test coverage before we review PR's. Would you be willing to add tests covering all fields changed where a "/" is now allowable, and make sure they still come out with the correct mount paths? Also, the docs may need to be updated (I'm not sure).

Happy to circle back and give a review when that's done. Thanks again!

@tyrannosaurus-becks ok 👌 I will work on it
By the way the name of the test functions for the okta part are not aligned with the Terraform standard (TestOkta... instead of TestAccOkta...)
I will rename that first : so do you want me to add it in the same PR or in another one ?

@rasta-rocket rasta-rocket force-pushed the okta_auth_backend_group_allow_slash branch from 53ee7da to 9872f84 Compare March 4, 2020 11:03
@ghost ghost added size/L and removed size/XS labels Mar 4, 2020
@rasta-rocket
Copy link
Contributor Author

@tyrannosaurus-becks I have updated the PR.
It contains:

  • some refacto about the tests related to the okta part
  • a first test case for the TestAccOktaAuthBackendGroup

If it fits the need I will replicate this model to TestAccOktaAuthBackendUser and TestAccOktaAuthBackend

Let me know about it ;)

tyrannosaurus-becks
tyrannosaurus-becks approved these changes Mar 6, 2020
View reviewed changes
Copy link
Contributor

@tyrannosaurus-becks tyrannosaurus-becks left a comment

Choose a reason for hiding this comment

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

This looks great! Thank you so much!

@tyrannosaurus-becks tyrannosaurus-becks merged commit 4357ccb into hashicorp:master Mar 6, 2020
dandandy pushed a commit to dandandy/terraform-provider-vault that referenced this pull request Jun 17, 2021
@rasta-rocket
fix(okta_auth_backend_group): allow '/' in group_name (hashicorp#687)
7c6cbd4 
* fix(okta_auth_backend_group): allow '/' in group_name

* refacto(okta): test names to be compliant with TF standard

* refacto(okta): tests - factorize organization parameter

* feat(okta): add test for group names with '/' inside
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tyrannosaurus-becks tyrannosaurus-becks tyrannosaurus-becks approved these changes

Assignees

@tyrannosaurus-becks tyrannosaurus-becks

Labels
size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rasta-rocket @tyrannosaurus-becks