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(subnet): handle new error message for deleting subnets with attached resources #923

Merged
merged 1 commit into from
May 2, 2024

Conversation

apricote
Copy link
Member

@apricote apricote commented May 2, 2024

The error message we rely on was never part of the API contract. It is soon being changed to the new message I have added. This does not fix any immediate bug, but prepares the provider for the coming months.

…hed resources

The error message we rely on was never part of the API contract. It is
soon being changed to the new message I have added. This does not fix
any immediate bug, but prepares the provider for the coming months.
@apricote apricote requested a review from a team as a code owner May 2, 2024 10:53
@apricote apricote self-assigned this May 2, 2024
Copy link

codecov bot commented May 2, 2024

Codecov Report

Attention: Patch coverage is 60.00000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 69.74%. Comparing base (800a66c) to head (5af1d40).

Files Patch % Lines
internal/network/resource_subnet.go 60.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #923      +/-   ##
==========================================
- Coverage   69.76%   69.74%   -0.02%     
==========================================
  Files          63       63              
  Lines        7938     7940       +2     
==========================================
  Hits         5538     5538              
- Misses       1701     1703       +2     
  Partials      699      699              
Flag Coverage Δ
e2e 69.74% <60.00%> (-0.02%) ⬇️
unit 34.21% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines +172 to +173
if hcloud.IsError(err, hcloud.ErrorCodeServiceError) &&
(strings.Contains(err.Error(), "servers are attached") || strings.Contains(err.Error(), "network has attached resources")) {
Copy link
Member

Choose a reason for hiding this comment

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

This feels dirty, any plan to handle this with a not human/changeable error message.

Copy link
Member Author

Choose a reason for hiding this comment

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

I do agree, relying on the message is hacky and can (evidently) break from time to time. I do not see any nice workarounds for this though at this time.

Changing the error code would be a breaking change, as we and other implementers might rely on the current code.

@apricote apricote merged commit 932c47b into main May 2, 2024
6 checks passed
@apricote apricote deleted the subnet-error-message branch May 2, 2024 12:20
apricote pushed a commit that referenced this pull request May 3, 2024
🤖 I have created a release *beep* *boop*
---


##
[1.47.0](v1.46.1...v1.47.0)
(2024-05-02)


### Features

* test with Terraform 1.8
([#919](#919))
([800a66c](800a66c))


### Bug Fixes

* **ssh-key:** data inconsistency with empty label objects
([#922](#922))
([7e1bf2c](7e1bf2c)),
closes
[#921](#921)
* **subnet:** handle new error message for deleting subnets with
attached resources
([#923](#923))
([932c47b](932c47b))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
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