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

Add acceptance tests for teams #103

Merged
merged 2 commits into from
Feb 22, 2021
Merged

Add acceptance tests for teams #103

merged 2 commits into from
Feb 22, 2021

Conversation

chloeruka
Copy link
Contributor

This is one of the resources that does not yet have acceptance tests associated with it. In order to guard against regressions, I have added the following tests:

  • creating a team (with only the required attributes)
  • updating a team (changing its name, description and visibility)
  • importing a team

During this work I discovered an error in importing teams. In order to resolve this, I added the DefaultMemberRole value which was present in the schema to the TeamNode struct, and added a mapping to updateTeam, so this PR doubles as a bugfix for importing teams.

This is one of the resources that does not yet have acceptance tests
associated with it. In order to guard against regressions, I have added
the following tests:

* creating a team (with only the required attributes)
* updating a team (changing its name, description and visibility)
* importing a team

During this work I discovered an error in importing teams. In order to
resolve this, I added the DefaultMemberRole value which was present in
the schema to the TeamNode struct, and added a mapping to updateTeam, so
this PR doubles as a bugfix for import.

if string(query.Node.Team.Name) != resourceState.Primary.Attributes["name"] {
return fmt.Errorf("Team name in state doesn't match remote name")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

In our other resource tests I think we only checked computer properties in this function, and I also note that name is checked in testAccCheckTeamRemoteValues. Do we definitely want to check it here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably excessive, I'll remove it!

@yob
Copy link
Contributor

yob commented Feb 21, 2021

During this work I discovered an error in importing teams. In order to resolve this, I added the DefaultMemberRole value which was present in the schema to the TeamNode struct, and added a mapping to updateTeam, so this PR doubles as a bugfix for importing teams.

Nice, tests work!

Purely out of interest, could you describe this bug in a bit more detail? Did it manifest as an error when updating a team during terraform apply?

@chloeruka
Copy link
Contributor Author

chloeruka commented Feb 22, 2021

Purely out of interesting, could you describe this bug in a bit more detail? Did it manifest as an error when updating a team during terraform apply?

image

When importing a resource, it was failing on ImportStateVerify: true because the returned resource did not have the field DefaultMemberRole present in the definition of the resource:

resource "buildkite_team" "test" {
  name = "example"
  description = "a cool team of example"
  privacy = "VISIBLE"
  default_team = true
  default_member_role = "MEMBER" # this field
}

@yob
Copy link
Contributor

yob commented Feb 22, 2021

Oh, right. So the schema had default_member_role and the provider would include it in team updates/creates, but it would never read the value back from buildkite and update the state?

Cool, nice find ✨

@chloeruka chloeruka requested a review from yob February 22, 2021 02:00
Copy link
Contributor

@yob yob left a comment

Choose a reason for hiding this comment

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

ship it 👍

@chloeruka chloeruka merged commit 7d61a93 into main Feb 22, 2021
@chloeruka chloeruka deleted the acceptance-test-teams branch February 22, 2021 05:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants