-
Notifications
You must be signed in to change notification settings - Fork 24
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
Add IsSensitive
to AttributeSchema
#42
Conversation
3cde2d4
to
34f98c4
Compare
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.
Thanks for the PR!
We'll need to reflect this new field also in hover data:
hcl-lang/decoder/attribute_candidates.go
Line 30 in b62f3a9
func detailForAttribute(attr *schema.AttributeSchema) string { |
Also would you mind adding some test cases for the public CandidatesAtPos
here
hcl-lang/decoder/candidates_test.go
Lines 803 to 807 in b62f3a9
testCases := []struct { | |
name string | |
pos hcl.Pos | |
expectedCandidates lang.Candidates | |
}{ |
and similar for HoverAtPos
here
hcl-lang/decoder/hover_test.go
Lines 328 to 332 in b62f3a9
testCases := []struct { | |
name string | |
pos hcl.Pos | |
expectedData *lang.HoverData | |
}{ |
One case in each (with sensitive attribute) should suffice; if you want to be thorough you could add one with AttributeSchema
where Expr
is ObjectExpr
with sensitive attribute nested within, although I'm not sure what's the current handling of it as nested expressions aren't well supported yet - but it's still nice to have a test that reflects the current behaviour - and will fail (and be fixed) as part of implementing hashicorp/terraform-ls#496
As per conversation in hashicorp/terraform-schema#49 (comment) we should also add some tests which test |
So that sensitive attributes can be flagged.
34f98c4
to
b631d54
Compare
if attr.IsRequired { | ||
detail = "Required" | ||
details = append(details, "required") |
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 is a reasonable change, but just FYI there will be some existing test data in the need of update potentially in terraform-schema
and in terraform-ls
😅
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.
sounds exciting 🙈
So that sensitive attributes can be flagged.