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/resource: Additional Go documentation for TestCheck and TestMatch functions #911

Merged
merged 3 commits into from
Mar 24, 2022

Conversation

bflad
Copy link
Member

@bflad bflad commented Mar 22, 2022

Closes #885

@bflad bflad added the documentation Improvements or additions to documentation label Mar 22, 2022
@bflad bflad added this to the v2.13.0 milestone Mar 22, 2022
@bflad bflad requested a review from a team as a code owner March 22, 2022 16:54
Copy link
Contributor

@detro detro left a comment

Choose a reason for hiding this comment

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

This is AWESOMELY USEFUL. I myself have learned more about those functions by reading this doc.

I left a couple of comments, but are nitpicks and it's ok if you don't feel like it.

Comment on lines +787 to +788
// The key parameter is an attribute path in Terraform CLI 0.11 and earlier
// "flatmap" syntax. Keys start with the attribute name of a top-level
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to make references to what is now an "ancient" version of Terraform? Do we have a public doc explaining this "flatmap" syntax?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would love to reference something, but I'm not sure if there's any terraform.io documentation left on it. 🙁 I'll do some digging, but I'm doubtful. In place of finding that, I'm wondering if Go testable examples might help at least slightly here.

Copy link
Member Author

Choose a reason for hiding this comment

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

The closest thing I can find is: https://www.terraform.io/language/configuration-0-11/interpolation#attributes-of-other-resources, but I think linking to that would honestly be more confusing than anything. The flatmap bits were a detail of the previous state storage mechanism. I've uploaded some TestCheckFunc examples instead.

// values of a list or map attribute:
//
// - .{NUMBER}: List value at index, e.g. .0 to inspect the first element
// - .{KEY}: Map value at key, e.g. .example to inspect the example key value
Copy link
Contributor

Choose a reason for hiding this comment

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

It would feel more "complete" if, as part of the flow of what you are explaining here, and the 2 special keys you just explained, there was an example that built on the above examples.

Something that would show that for

  data "myprovider_thing" "example" {
    attributeName {
      k1 = "v1"
    }
  }

they could use this function like:

TestCheckResourceAttrSet("data.myprovider_thing.example", "attributeName.k1")

Does it make sense?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah totally -- I meant to implement Go testable examples for these as part of this update.

It's going to be a little awkward though, because its only recommended to implement these for Computed values, but I'll essentially have to show the configuration equivalent so it makes more sense looking at it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've uploaded some TestCheckFunc examples.

// exists in state for the given name/key combination. It is useful when
// testing that computed values were set, when it is not possible to
// know ahead of time what the values will be.
// TestCheckResourceAttrSet ensures any value exists in the state for the
Copy link
Contributor

Choose a reason for hiding this comment

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

It took me reading this twice to realise that the Set suffix here means "that it is set" and not that it usable with TypeSet.
🤦‍♀️

Nitpick: Probably too late to change it to IsSet suffix?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that's certainly a good naming callout, although that ship sailed years ago. 🚢 I'd also vote for TestCheckResourceAttrAnyValue or something (and alias TestCheckNoResourceAttr to TestCheckResourceAttrNoValue) if we were to change it/them. It is not worth the developer burden at this point though, in my opinion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Of course. Naming, especially that lasts the test of time, is hard.

// for sets.
// - .{KEY}: Map value at key, e.g. .example to inspect the example key value
// - .#: Number of elements in list or set
// - .%: Number of elements in map
Copy link
Contributor

Choose a reason for hiding this comment

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

As above, an "end to end" example would put a nice bow here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've uploaded some TestCheckFunc examples.

// values of a list or map attribute:
//
// - .{NUMBER}: List value at index, e.g. .0 to inspect the first element
// - .{KEY}: Map value at key, e.g. .example to inspect the example key value
Copy link
Contributor

Choose a reason for hiding this comment

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

About the example, as above.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've uploaded some TestCheckFunc examples.

// for sets.
// - .{KEY}: Map value at key, e.g. .example to inspect the example key value
// - .#: Number of elements in list or set
// - .%: Number of elements in map
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Copy link
Member Author

Choose a reason for hiding this comment

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

I've uploaded some TestCheckFunc examples.

// for sets.
// - .{KEY}: Map value at key, e.g. .example to inspect the example key value
// - .#: Number of elements in list or set
// - .%: Number of elements in map
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Copy link
Member Author

Choose a reason for hiding this comment

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

I've uploaded some TestCheckFunc examples.

Copy link
Contributor

@detro detro left a comment

Choose a reason for hiding this comment

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

The use of Go testable examples is a great solution, much better than keeping it in code.

In Rust there is something similar: the compiler can be told that part of the doc is an executable example and so it handles that in a similar fashion to here.

@detro
Copy link
Contributor

detro commented Mar 24, 2022

Screenshot 2022-03-24 at 11 42 51
I was curious to see the test examples in action, so I ran godoc locally.

I think you might need to tweak the formatting a bit, as the godoc renderer is getting a bit confused in some points.

@detro
Copy link
Contributor

detro commented Mar 24, 2022

But godoc formatting aside, this is excellent.

@bflad
Copy link
Member Author

bflad commented Mar 24, 2022

@detro THANK YOU! I always forget the correct list formatting bits in Go doc (the current thing today is to just treat them as a code block). I've pushed up the changes to that and it looks much, much better.

Screen Shot 2022-03-24 at 10 04 54 AM

Excited to see the proposal golang/go#51082 land yesterday, which will definitely make this documentation easier to write in the future. 🎉

Aside: I also forget about the title formatting, which would probably help with some of the longer documentation bits, but we can always follow up with those at any time.

@bflad bflad merged commit 0045c35 into main Mar 24, 2022
@bflad bflad deleted the bflad-helper-resource-TestCheck-docs branch March 24, 2022 14:13
bflad added a commit that referenced this pull request Mar 24, 2022
Reference: #911

Additional glow up for the `TypeSet` functions, similar to the others.
bflad added a commit that referenced this pull request Mar 28, 2022
#916)

Reference: #911

Additional glow up for the `TypeSet` functions, similar to the others.
@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 Apr 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

testCheckResourceAttrSet() is broken for all but TypeString attributes
2 participants