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

ESD-25205: Fix concurrency issue in association resources #425

Merged
merged 4 commits into from
Jan 4, 2023

Conversation

sergiught
Copy link
Contributor

@sergiught sergiught commented Jan 4, 2023

🔧 Changes

All the association resources are prone to concurrency issues when needing to operate on the same entity. In this PR we are introducing a global mutex to prevent such issues.

📚 References

🔬 Testing

📝 Checklist

  • All new/changed/fixed functionality is covered by tests (or N/A)
  • I have added documentation for all new/changed functionality (or N/A)

@sergiught sergiught self-assigned this Jan 4, 2023
connection, err := api.Connection.Read(connectionID)
if err != nil {
if mErr, ok := err.(management.Error); ok && mErr.Status() == http.StatusNotFound {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

404 checks should not happen on CREATE to prevent Root resource was present, but now absent errors.

@@ -159,6 +162,10 @@ func readOrganizationMember(ctx context.Context, d *schema.ResourceData, m inter

roles, err := api.Organization.MemberRoles(orgID, userID)
if err != nil {
if err, ok := err.(management.Error); ok && err.Status() == http.StatusNotFound {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This 404 check was missed on this read func.

@sergiught sergiught marked this pull request as ready for review January 4, 2023 21:06
@sergiught sergiught requested a review from a team as a code owner January 4, 2023 21:06
@codecov-commenter
Copy link

Codecov Report

Base: 86.79% // Head: 86.87% // Increases project coverage by +0.07% 🎉

Coverage data is based on head (704cd91) compared to base (33f3c12).
Patch coverage: 87.69% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #425      +/-   ##
==========================================
+ Coverage   86.79%   86.87%   +0.07%     
==========================================
  Files          41       42       +1     
  Lines        9036     9091      +55     
==========================================
+ Hits         7843     7898      +55     
  Misses        924      924              
  Partials      269      269              
Impacted Files Coverage Δ
internal/provider/provider.go 59.59% <ø> (ø)
...provider/resource_auth0_organization_connection.go 87.14% <75.00%> (-0.86%) ⬇️
...nal/provider/resource_auth0_organization_member.go 80.51% <76.47%> (-0.20%) ⬇️
internal/mutex/mutex.go 100.00% <100.00%> (ø)
...ernal/provider/resource_auth0_connection_client.go 79.71% <100.00%> (+4.71%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@sergiught sergiught force-pushed the patch/ESD-25205-404s-root-stuff-error branch from 704cd91 to c21ef59 Compare January 4, 2023 21:08
Copy link
Contributor

@willvedd willvedd left a comment

Choose a reason for hiding this comment

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

@sergiught and I talked this over. We agree this is a push in the right direction and reduces esoteric errors. A similar precedent is already set by the AWS provider.

@sergiught sergiught merged commit a10d01c into main Jan 4, 2023
@sergiught sergiught deleted the patch/ESD-25205-404s-root-stuff-error branch January 4, 2023 21:45
@willvedd willvedd mentioned this pull request Jun 12, 2023
2 tasks
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.

3 participants