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

feat: push connection speed validation down to the API #610

Merged
merged 5 commits into from
Mar 4, 2024

Conversation

ctreatma
Copy link
Contributor

@ctreatma ctreatma commented Mar 4, 2024

Previously, we maintained a list of valid connection speed values in the terraform provider in order to steer users towards selecting a valid connection speed. As shown by #595, validating values in the provider has the potential to break when the list of connection speed values supported by the API changes.

This updates the code that converts speed values between integer and string representations (i.e., the code that knows that "50Mbps" is the same as 500000000) to convert (roughly) arbitrary values. This means that new values are immediately "supported" by the provider, but it also means that invalid values will not be flagged until apply.

The test change in this PR is sufficient to resolve the issue introduced by merging #605 without any other change to provider behavior.

Closes #609

@ctreatma ctreatma marked this pull request as ready for review March 4, 2024 15:58
@ctreatma ctreatma requested review from t0mk and a team as code owners March 4, 2024 15:58
@codecov-commenter
Copy link

codecov-commenter commented Mar 4, 2024

Codecov Report

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

Project coverage is 60.43%. Comparing base (a09e337) to head (72c0000).

Files Patch % Lines
internal/resources/metal/connection/speed.go 95.23% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##             main     #610       +/-   ##
===========================================
+ Coverage   41.74%   60.43%   +18.69%     
===========================================
  Files         102      102               
  Lines       18551    18554        +3     
===========================================
+ Hits         7745    11214     +3469     
+ Misses      10600     6830     -3770     
- Partials      206      510      +304     

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

@@ -50,7 +50,7 @@ func dataSourceSchema(ctx context.Context) schema.Schema {
Computed: true,
},
"speed": schema.StringAttribute{
Description: fmt.Sprintf("Port speed. Possible values are %s", allowedSpeedsString()),
Description: fmt.Sprintf("Port speed. Required for a_side connections. Values will be in the format '<number>Mbps' or '<number>Gpbs'"),
Copy link
Member

Choose a reason for hiding this comment

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

The .md should reflect the change. Perhaps we could give some examples speeds in the description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the docs, but there was already some drift between the description in docs and the description in code; we probably should pick one and use it in both places, but which one to pick is an exercise left to the reviewer.

Copy link
Contributor Author

@ctreatma ctreatma Mar 4, 2024

Choose a reason for hiding this comment

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

For comparison:
- description in resource docs
- description in resource code

Went ahead and gave them the same description in docs and code.

@ctreatma ctreatma force-pushed the shared-connection-vlans branch from ab4e9d0 to 4870ab8 Compare March 4, 2024 18:23
@ctreatma ctreatma merged commit 2f918dd into main Mar 4, 2024
4 of 5 checks passed
@ctreatma ctreatma deleted the shared-connection-vlans branch March 4, 2024 21:59
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.

TestSpeedConversion test is failing on 100gbps int/str mismatch
4 participants