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: verify & set parent team after creation #1449

Merged
merged 7 commits into from
Jan 11, 2023

Conversation

reiniertimmer
Copy link
Contributor

@reiniertimmer reiniertimmer commented Dec 22, 2022

Resolves #1167

Behavior

Before the change?

Using a GitHub App, the parent team is not set (when the parent team was not created by the GitHub App). Effectively, the created team will then be at the root level with a pending approval.

image

However, the GitHub App should be allowed to nest under parent teams, this just needs an additional call to the REST API update team endpoint.

After the change?

When adding a team with a parent team specified, it will now work the same with GitHub Apps as it works with PATs.

Other information

This change only adds one check:

  • if a parent was requested, but the nesting was not created, this change will just re-attempt to create the nesting. This may still mean that the nesting will not be done properly.
  • existing tests are only for testing the integration with a PAT. I did not add an additional case using a GitHub App. Maybe that's something can be done separately?

Pull request checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Added the appropriate label for the given change

Does this introduce a breaking change?

Should not be a breaking change.

  • Yes (Please add the Type: Breaking change label)
  • No

Pull request type

  • Bugfix: Type: Bug

@reiniertimmer reiniertimmer changed the title explicitly check & set parent team after creation fix: verify & set parent team after creation Dec 22, 2022
@nickfloyd nickfloyd added the Type: Bug Something isn't working as documented label Dec 30, 2022
@kfcampbell
Copy link
Member

@reiniertimmer would you mind adding a brief code comment describing the need and circumstances around the extra call? I think I'll be confused when looking at this again in the future and a description may be helpful.

@reiniertimmer
Copy link
Contributor Author

@kfcampbell of course I can do that. Will do that as soon as I have the opportunity.

I must say that the whole behavioural difference in the API is a bit unexpected.

The additional call to set the parent is also not guaranteed to succeed (permission-wise, even with a PAT) which makes this something of a best-effort attempt.

I'll see if I can catch that in a short but meaningful comment 😊

@reiniertimmer
Copy link
Contributor Author

@kfcampbell I added a comment that hopefully explains what is going on.

This PR resolves an issue specific to using GitHub Apps. Since the contribution section only describes running the provider with a PAT, I'm not sure when (and if) such issues can be caught automatically. Using GitHub Apps for automation also seems like a logical choice in some cases.

Is there anything that can be done to improve the testing with GitHub App tokens as well? Or is that effort already being started here? #1414

@kfcampbell
Copy link
Member

Is there anything that can be done to improve the testing with GitHub App tokens as well? Or is that effort already being started here? #1414

The will to do so is there but unfortunately the effort hasn't been started yet. I agree that testing with GitHub Apps would be a great idea and it's something we should do in the future.

Copy link
Member

@kfcampbell kfcampbell left a comment

Choose a reason for hiding this comment

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

I've confirmed that the relevant integration tests are still passing locally on this branch. I'll get this merged and released sometime in the near future; hopefully it'll help ease some of the errors users are seeing.

@kfcampbell kfcampbell merged commit 4644907 into integrations:main Jan 11, 2023
avidspartan1 pushed a commit to avidspartan1/terraform-provider-github that referenced this pull request Feb 5, 2024
* explicitly check & set parent team after creation

* Added comment about setting a parent team

* Fix comment formatting

* comment formatting

Co-authored-by: Nick Floyd <139819+nickfloyd@users.noreply.github.com>
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
Labels
Type: Bug Something isn't working as documented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

team with parent_team_id created at root of org
3 participants