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

adds entitlment error message for creating Teams #418

Merged
merged 2 commits into from
Jan 25, 2022
Merged

adds entitlment error message for creating Teams #418

merged 2 commits into from
Jan 25, 2022

Conversation

mfhodges
Copy link
Contributor

@mfhodges mfhodges commented Jan 24, 2022

Description

This PR resolves Issue 171. Previously when a User on the Free Tier tried to create a team using the tfe provider, the CLI would return an error message like : Error: Error creating team my-team-name for organization raoul-staging: resource not found. This error does not explain to the user that they cannot create a team because the Free tier does not have the proper entitlements.

These changes check entitlements after the ErrResourceNotFound error and if there is an error surfaces an error message like: Error creating team my-team-name for organization raoul-staging: missing entitlements to create teams

Testing plan

  1. Run tfe provider locally against staging.
  2. Create an Organization and set the Feature Set to Free tier.
  3. Using the CLI run terraform apply to create a team in the Org - This should surface an error due to entitlements
  4. Set the Organization to Business and re-run the apply - This should not surface any error.

@hashicorp-cla
Copy link

hashicorp-cla commented Jan 24, 2022

CLA assistant check
All committers have signed the CLA.

@mfhodges mfhodges marked this pull request as ready for review January 24, 2022 15:31
@mfhodges mfhodges requested a review from a team as a code owner January 24, 2022 15:31
Copy link
Contributor

@sebasslash sebasslash left a comment

Choose a reason for hiding this comment

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

I've pulled your changes locally and have tested they work as intended. Heck yeah 🔥

"Error creating team %s for organization %s: %v", name, organization, err)
if err == tfe.ErrResourceNotFound {
entitlements, _ := tfeClient.Organizations.Entitlements(ctx, organization)
if !entitlements.Teams {
Copy link
Collaborator

Choose a reason for hiding this comment

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

a panic might happen here if entitlements is nil, so it may be best to check for the error return value here before dereferencing entitlements

It may be interesting to eventually pull entitlements up front so that all resources could short circuit with a helpful error message some day. I didn't know about this API. I'll look into it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you know how I could test this edge case in staging? I'm new to the product so it's not clear to me how entitlements could be nil

Copy link
Contributor

Choose a reason for hiding this comment

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

It could be something as simple as the API returns a 500 or a 503. If an organization has zero entitlements, that would be a huge problem and beyond the scope of the provider.

I don't think an explicit test is necessary; simply having a block that handles the error and returns a clear error msg is sufficient.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would just fall through and let the 404 error be returned

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gotcha! that makes a lot of sense. thanks for clarifying that 😊. Just pushed changes for this feedback

Copy link
Collaborator

@brandonc brandonc left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this!

@mfhodges mfhodges merged commit 320684b into main Jan 25, 2022
@mfhodges mfhodges deleted the maddy/test branch January 25, 2022 13:53
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.

Unclear error when attempting to add teams to free accounts
4 participants