-
Notifications
You must be signed in to change notification settings - Fork 11
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
Audit error reporting #62
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @fantapop)
"Could not add network allow list in serverless cluster", | ||
fmt.Sprintf("Network allow list is a feature of dedicated cluster: %s", formatAPIErrorMessage(err)), | ||
"Could not add network allowlist in serverless cluster", | ||
"Network allowlists are only supported with dedicated clusters", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This message makes me worried. Its not obvious how the lack of Serverless config equates to a network allow list error. I'm also worried that whatever holds true now that makes this work may change in the future and there'd be no good way to catch that this is no longer correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the presence of a serverless config, not lack. This is a validation step, and in a future version, this will happen at plan time. That plan-time validation (and corresponding dry run) will happen way before we add these sorts of network tools to serverless clusters.
) | ||
return | ||
} else if cluster.CloudProvider != client.APICLOUDPROVIDER_AWS { | ||
resp.Diagnostics.AddError( | ||
"Incompatible cluster cloud provider", | ||
"Private endpoint services are only available for AWS clusters.", | ||
"Private endpoint services are only available for AWS clusters", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reformat seems fine but I think we should consider updating the language to reflect that this is temporary until its supported on other clouds. For example:
"Private endpoint services are currently only available for AWS clusters"
Doesn't need to be part of this review. @ianjevans
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated!
Attempted to audit all calls to diagnostics.AddError to ensure that the error messages are accurate and follow a consistent formatting scheme, according to https://developer.hashicorp.com/terraform/plugin/log/writing
6daaeae
to
61a4389
Compare
Previously, erademacher (Evan Rademacher) wrote…
oh I see, this is in the creation of the allow list itself. Makes sense. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 3 of 5 files reviewed, all discussions resolved (waiting on @ianjevans and @ryanluu12345)
TFTRs! |
Attempted to audit all calls to diagnostics.AddError to ensure that the error messages are accurate and follow a consistent formatting scheme, according to https://developer.hashicorp.com/terraform/plugin/log/writing. Standardized on "allowlist" instead of "allow list".
Also cleaned up some obsolete comments and validation checks.
Add a description of the problem this PR addresses and an overview of how this PR works.
This change is![Reviewable](https://camo.githubusercontent.com/23b05f5fb48215c989e92cc44cf6512512d083132bd3daf689867c8d9d386888/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)