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

Fix github_team_members: Support github_team_members role edits #1216

Merged
merged 2 commits into from
Aug 23, 2022

Conversation

csainty
Copy link
Contributor

@csainty csainty commented Jul 8, 2022

When a member's role is changed, only the delete operation is run.
The code clearly sets both delete and create on lines 140:141, but then uses continue to loop the iteration after running just one of them.

From what I can work out, the test case already checks this and is failing. I have therefore left it unchanged and it should start passing.

I say should because in my local environment I had to edit the test to not mess with org membership, otherwise my collaborator account was pending invitation to the org and failing the test because of that. I am thinking your test setup approves those invitations automatically.
With that change made this PR takes the test from FAIL to PASS for me.

@csainty
Copy link
Contributor Author

csainty commented Jul 17, 2022

@stawik-mesa as the original author, are you willing to check my thinking here and confirm what I am seeing? 🙏

@stawik-mesa
Copy link
Contributor

@csainty I think you are right. Probably I had not enough sleep at the time I implemented it.

@csainty
Copy link
Contributor Author

csainty commented Jul 19, 2022

I think you are right. Probably I had not enough sleep at the time I implemented it.

Don't beat yourself up, simple mistake.

Now let's tag @jcudit and @kfcampbell as the original approvers and see if they can weigh in.

@kfcampbell
Copy link
Member

I've been able to reproduce the failure and subsequent test passing following @csainty's advice locally. I do not believe that the integration tests are being run correctly in CI. I'll get this merged shortly and released soon, and prioritize work on our integration testing setup.

@kfcampbell kfcampbell merged commit 2c15b55 into integrations:main Aug 23, 2022
@csainty csainty deleted the fix/github_team_members branch August 24, 2022 00:22
kazaker pushed a commit to auto1-oss/terraform-provider-github that referenced this pull request Dec 28, 2022
Co-authored-by: Keegan Campbell <me@kfcampbell.com>
avidspartan1 pushed a commit to avidspartan1/terraform-provider-github that referenced this pull request Feb 5, 2024
Co-authored-by: Keegan Campbell <me@kfcampbell.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants