SUP-1307 Implement Pipeline Team Resource #351
Conversation
…line-resource-to-framework
|
I think what we need to apply to this PR now is:
This means that terraform will no longer do anything about new teams it finds but that is a defacto standard for other providers and how this will work in the future once the |
buildkite/resource_pipeline_team.go
Outdated
| tpState.AccessLevel = types.StringValue(string(tpNode.PipelineAccessLevel)) | ||
| } | ||
|
|
||
| func findTeamInPipelineNode(client *graphql.Client, tpState *pipelineTeamResourceModel) bool { |
There was a problem hiding this comment.
This looks unused now. Can it be removed?
There was a problem hiding this comment.
Same as the issue in line 74 comment, the unused function got reverted in this commit Fix: Missing } on setPipelineModel and removed unused function blocks. I will make sure it got reverted to that.
| Steps: []resource.TestStep{ | ||
| { | ||
| Config: testAccPipelineTeamConfigBasic(teamName, "READ_ONLY"), | ||
| ExpectNonEmptyPlan: true, |
There was a problem hiding this comment.
Why does this test expect a non-empty plan? I would expect this test case to create the 3 resources and then not have any other changes to make after that
There was a problem hiding this comment.
After running the tests, a terraform refresh is run, and this generates a non empty plan because the pipeline resource gets updated with the team block
There was a problem hiding this comment.
Yeah thats right, but thats not what we want. We want the pipeline.team block to ignore the appearance of new teams
| Steps: []resource.TestStep{ | ||
| { | ||
| Config: testAccPipelineTeamConfigBasic(teamName, "BUILD_AND_READ"), | ||
| ExpectNonEmptyPlan: true, |
There was a problem hiding this comment.
Same with this. I'd expect an empty plan here
There was a problem hiding this comment.
same as above reason for adding the ExpectNonEmptyPlan
| }, | ||
| }) | ||
| } | ||
|
|
There was a problem hiding this comment.
I think it would be good to have a test where terraform creates the pipeline_team but then its removed on the API side. To ensure that it removes it from state and plans to recreate it
buildkite/resource_pipeline_test.go
Outdated
| @@ -581,9 +581,11 @@ func TestAccPipeline_remove_team(t *testing.T) { | |||
| // Quick check to confirm the local state is correct before we update it | |||
| resource.TestCheckResourceAttr("buildkite_pipeline.foobar", "name", "Test Pipeline foo"), | |||
| ), | |||
| ImportStateVerifyIgnore: []string{"slug"}, | |||
There was a problem hiding this comment.
Is this what you were talking about at the standup?
What was the error showing up? I'm wondering if there is something else going wrong here and adding this is masking it
There was a problem hiding this comment.
Yes, this is what we discussed during standup yesterday. Interestingly, the test no longer fails locally if I remove ImportStateVerifyIgnore. Let me revert that change.
PR checklist:
docs/updatedexamples/(useful to demo new field/resource)CHANGELOG.mdupdated with pending release informationTests to fix (currently skipped)