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

Suppress or de-prioritise bad-practice constraints for some attributes #1473

Open
2 tasks
radeksimko opened this issue Oct 24, 2023 · 0 comments
Open
2 tasks

Comments

@radeksimko
Copy link
Member

Context

Currently, we use the schema to drive both the validation (which decided what is or isn't valid) and also to provide completion, hover and other more "positive" features.

We have had to make changes to the schema such that we account for more edge cases that would otherwise be considered entirely invalid during validation. This however made them appear as entirely valid, when in reality we want to be discouraging users from them.

Examples

terraform {
  required_providers {
    aws = "~> 5.22"
  }
}

Screenshot 2023-10-24 at 15 57 59

^ provider requirements with implied source address are a bad practice for any recent Terraform versions as it's not obvious which exact provider does the configuration require (e.g. hashicorp/aws or foobar/aws or customregistry.io/foo/aws?). Therefore, object should always be preferred here

variable "baz" {
  type = set(string)
}
variable "bar" {
  type = object({})
}

resource "null_resource" "example" {
  for_each = var.bar
}

Screenshot 2023-10-24 at 15 56 22

^ objects are strictly speaking allowed in for_each but in the majority of cases this is just a result of misunderstanding of types where users would be better off with set(string) or list(...) rather than an object. Therefore, set() should be almost always a better option in favour of object().

Proposal

  • De-prioritise or suppress bad-practice options in completion (i.e. sort them to be near the bottom)
  • Separate bad-practice constraints in hover (e.g. render them on a separate bottom line as "also supported" or something like that)

We could also entirely suppress these options, but this will depend on our confidence about it being bad practice. The first example with required_providers has high confidence, but the second one still has some unfortunate valid use cases.

Implementation Notes

Introduce a new interface, similar to schema.ConstraintWithHoverData or schema.Validatable, e.g.

type Suppressable interface {
	IsSuppressed() bool
	IsDeprioritised() bool
}

Implement the interface into the relevant constraints, such as schema.LiteralType and schema.AnyExpression.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant