-
Notifications
You must be signed in to change notification settings - Fork 156
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
Workspace additions #287
Workspace additions #287
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.
Two small things, then this will be ready for ✅ and merge!
tfe/data_source_workspace_test.go
Outdated
@@ -47,6 +47,10 @@ func TestAccTFEWorkspaceDataSource_basic(t *testing.T) { | |||
"data.tfe_workspace.foobar", "working_directory", "terraform/test"), | |||
|
|||
resource.TestCheckResourceAttrSet("data.tfe_workspace.foobar", "external_id"), | |||
resource.TestCheckResourceAttrSet("data.tfe_workspace.foobar", "resource_count"), |
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.
I'm curious why these use TestCheckResourceAttrSet
instead of TestCheckResourceAttr
! The latter would let us verify that the attribute actually has the value we expect as well as checking that it's set.
Granted, it's a somewhat academic difference in this case, since the test workspace won't have any runs and therefore the values will all be zero 😆
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.
I think I used TestCheckResourceAttrSet
because I didn't have a way to test that they'd be anything but the zero-value. I can change to TestCheckResourceAttr
if you think it would be better
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.
That makes sense! Not a huge deal either way, but I'd lean towards using TestCheckResourceAttr
since we do know the value - unlike, say, checking the workspace's ID, where we have no way of knowing what the external ID will be before it's generated.
I went ahead and pushed up a commit with that change: 0de74c2. If you're ok with it, I'll cherry-pick that commit into this branch and then smash that merge button.
Thanks! feel free to merge |
Co-authored-by: CJ Horton <17039873+radditude@users.noreply.github.com>
0157024
to
41c36f8
Compare
Description
Adds support for new fields in the workspace object
Testing plan
Integration tests should capture these changes
External links
Output from acceptance tests
I wasn't able to get these working locally. Perhaps we can use CI