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

DXCDT-458: Handling 404 errors in data sources #698

Merged
merged 7 commits into from
Jul 10, 2023

Conversation

willvedd
Copy link
Contributor

@willvedd willvedd commented Jul 10, 2023

🔧 Changes

As reported by #604 , there is inconsistent handling of 404 errors within data sources. This is attributed to two reasons:

  • A resource's readFunc being repurposed for the correlating data source which normally smooths-over the 404 error
  • Occasional bespoke data source read functions due to alternative identifiers to fetch by (ex: ID, name, etc).

This PR takes a comprehensive look at each data source and modifies them to handle 404s appropriately.

The approach taken in cases where a resource's read function is repurposed is to wrap that function in a 404 checking function. This was chosen instead of a complete codebase refactor that abstracts the 404 handling for each read funciton.

📚 References

Related Github issue: #604

🔬 Testing

Added tests for each data source.

📝 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)

@willvedd willvedd requested a review from a team as a code owner July 10, 2023 15:57
Comment on lines -82 to -85
if mErr, ok := err.(management.Error); ok && mErr.Status() == http.StatusNotFound {
data.SetId("")
return nil
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted because we want to handle 404s in the data source.

Comment on lines -52 to -55
if mErr, ok := err.(management.Error); ok && mErr.Status() == http.StatusNotFound {
data.SetId("")
return nil
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted because we want to handle 404s in the data source. This fixes #604 .

Comment on lines +13 to +22
func CheckFor404Error(ctx context.Context, readFunc func(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Diagnostics, d *schema.ResourceData, m interface{}) diag.Diagnostics {
diags := readFunc(ctx, d, m)
if diags != nil {
return diags
}
if d.Id() == "" {
return diag.FromErr(errors.New("no resource found with that identifier (404)"))
}
return nil
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aside from abstracting-out each read function and handling the errors within the respective instances (resource vs data source), detecting an empty ID is the simplest solution, albeit not the most elegant.

This is mostly just a proposal of what we could do but at this point I don't think doing a wider refactor would be that much more effort and establishes a better code pattern.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is already a solid n+1, we can refactor further after everything's in v1 and we have time. 👍🏻

@willvedd willvedd merged commit f79ada1 into v1 Jul 10, 2023
@willvedd willvedd deleted the DXCDT-458-handle-404-data-sources branch July 10, 2023 19:36
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.

2 participants