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

helper/schema: Adjusted validation error messages #755

Merged
merged 1 commit into from
Apr 30, 2021

Conversation

apparentlymart
Copy link
Contributor

Some of the validation error messages were written with the provider developer audience rather than the end-user audience in mind, and so in helping various folks in the community forum, etc I've occasionally seen folks be confused as to what these messages mean and what to do about them.

The main cases I came here to address were:

  • The error messages whose summaries directly mention field names from the schema.Schema type, which is not something we expect end-sers to be familiar with.
  • The "computed attributes cannot be set" error, because the terminology "computed" doesn't appear in our user-facing documentation. Since we don't actually really have a specific and consistent term for that, rather than introducing one here I instead made it a more plain description of the situation: an "unconfigurable attribute".

Since I was here anyway I also did some general review of the terminology used and adjusted some things that were not necessarily confusing but that were using different terminology than Terraform Core would use for a similar problem. However, for many of them I didn't change them heavily because in many cases these validation errors are masked by equivalent schema-based validation in Terraform Core itself anyway, and so these can often be seen only by Terraform v0.10/v0.11 users.

@apparentlymart apparentlymart added enhancement New feature or request ui UI specific change or change which requires significant UI changes labels Apr 26, 2021
@apparentlymart apparentlymart requested a review from a team April 26, 2021 17:09
@apparentlymart apparentlymart self-assigned this Apr 26, 2021
Copy link
Contributor

@paddycarver paddycarver left a comment

Choose a reason for hiding this comment

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

I'm noticing some inconsistencies here about when "attribute" is used and when "argument" is used. Is there an underlying system at work here that I'm not picking up on?

@@ -1453,7 +1453,7 @@ func (m schemaMap) validate(
if err != nil {
return append(diags, diag.Diagnostic{
Severity: diag.Error,
Summary: "ExactlyOne",
Summary: "Missing required argument",
Copy link
Contributor

Choose a reason for hiding this comment

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

This may not be strictly correct, as I believe this would error if none of the arguments were selected, or if more than one of the arguments were selected.

Suggested change
Summary: "Missing required argument",
Summary: "Invalid number of arguments",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh yes, true: this can actually be describing two different problems:

  • You didn't set any of the arguments in the set.
  • You set more than one of the arguments in the set.

I think I'd prefer to change this to "Invalid combination of arguments", because "number" here makes it feel to me like there's the wrong number of arguments in this block in total, rather than the wrong number of arguments within this particular set of arguments that isn't directly visible to the user except in this error message. I will change it, though!

@apparentlymart
Copy link
Contributor Author

apparentlymart commented Apr 27, 2021

Regarding argument vs. attribute: "attribute" is the general term for something you could access on the resulting object using attribute syntax, like .foo, whereas "argument" describes the subset of attributes that you can set in the configuration.

We adopted this terminology in response to the large amount of existing precedent in provider documentation, like in aws_instance where we have the heading "Argument Reference" talking about what users can set and the heading "Attributes Reference" which defines various other attributes "in addition to all arguments above". This terminology is now reflected in the main Terraform Language documentation, in Resource Arguments and in Accessing Resource Attributes.

This also speaks a bit to my comment about referring to the "unconfigurable attributes" in my initial writeup: we don't have any established terminology in the user model for what's left if you subtract "arguments" from "attributes", and so I coined "unconfigurable attributes" here as an informal way to refer to those.

Some of the validation error messages were written with the provider
developer audience rather than the end-user audience in mind, and so in
helping various folks in the community forum, etc I've occasionally seen
folks be confused as to what these messages mean and what to do about
them.

Since I was here anyway I also did some general review of the terminology
used and adjusted some things that were not necessarily confusing but that
were using different terminology than Terraform Core would use for a
similar problem. However, for many of them I didn't change them heavily
because in many cases these validation errors are masked by equivalent
schema-based validation in Terraform Core itself anyway, and so these can
often be seen only by Terraform v0.10/v0.11 users.
Copy link
Contributor

@paddycarver paddycarver left a comment

Choose a reason for hiding this comment

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

Thanks @apparentlymart, I think this is much clearer for users and I'm happy to see the consistency in terms get propagated :)

@paddycarver paddycarver merged commit 9321fe1 into main Apr 30, 2021
@paddycarver paddycarver deleted the f-validate-messages branch April 30, 2021 12:38
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request ui UI specific change or change which requires significant UI changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants