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: mssql storage type extra #185149472 #1061

Closed
wants to merge 5 commits into from

Conversation

fnaranjo-vmw
Copy link
Contributor

@fnaranjo-vmw fnaranjo-vmw commented Aug 7, 2023

This PR is related to #1060.

It suggests a different approach to the iops and storage_type interdependency.
Instead of masking some invalid combinations from the user, let every invalid
combination fail with descriptive messages.

This PR requires a team discussion and the approval of the whole team to be merged.

Checklist:

@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/185779669

The labels on this github issue will be updated when the story is started.

@fnaranjo-vmw fnaranjo-vmw changed the title Feat mssql storage type extra #185149472 feat: mssql storage type extra #185149472 Aug 7, 2023
[#185149472]

So far, other services preferred ignoring iops field when storage
type was not io1 or gp3. This implementation raises error instead.

The reasoning is to follow the principle of least surprise. If a
specific combination is invalid, enforce users to clearly specify
correct values.

There is some level of surprise though, because iops is 3000 by
default, so a customer might not be aware of this and still get
the error. However, even in that case, it might be useful to
surface the error to make the default value explicit and evident.
@fnaranjo-vmw fnaranjo-vmw force-pushed the feat-mssql-storage-type-extra-#185149472 branch from 0ed5016 to bad6fa7 Compare August 7, 2023 17:15
[#185149472]

Terraform already checks more combinations than us, and do it
fast without having to wait for an error from th IAAS.
Removing these redundant and incomplete validations.

Also allowed iops to be set to null. This is to allow customers
to override the default iops value, in case they want to use
a non-iops based storage_type such as gp2 or standard.
@fnaranjo-vmw fnaranjo-vmw force-pushed the feat-mssql-storage-type-extra-#185149472 branch from 830260d to 75e214e Compare August 9, 2023 16:10
[#185149472]

They must have ended-up in the wrong test by accident.
@fnaranjo-vmw
Copy link
Contributor Author

Added @servicesenablement as a reviewer to prevent this PR from appearing in Slack Code Review bot messages.

@fnaranjo-vmw fnaranjo-vmw added enhancement New feature or request progressive-improvement and removed enhancement New feature or request labels Aug 10, 2023
@fnaranjo-vmw fnaranjo-vmw marked this pull request as draft August 10, 2023 12:27
@fnaranjo-vmw fnaranjo-vmw removed the request for review from servicesenablement August 10, 2023 12:28
@blgm
Copy link
Member

blgm commented Jun 7, 2024

Closed due to no recent activity

@blgm blgm closed this Jun 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants