-
Notifications
You must be signed in to change notification settings - Fork 770
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
addition of team sync support #400
Conversation
Hi @anGie44 I'm not a contributor in this provider, but should not you create also the documentation pages in /website/docs/r|d ? Thanks! |
ah yes great catch @jopedros, thank you! i'll make the addition. |
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.
Looks really good! A few comments, but it's a great first PR! Nice work!
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.
Great changes! Looks good, thanks!
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.
LGTM! Great work!
@megan07 I was finally able to test the data/resource w/my test github enterprise cloud org configured w/SAML and Azure AD. The changes I pushed up reflect some bugs I found but the API calls are still error prone -- filed the issue/PR here: google/go-github#1484 In the meantime, the tests catch the 404 errors/missing state:
|
@anGie44 - https://github.com/google/go-github/releases/tag/v31.0.0 has been released, as requested. Thank you for your contributions! They are greatly appreciated! |
Note: these changes (and subsequent changes to handle API calls) will be contingent on upgrade to |
e32a61c
to
fd40520
Compare
f1cb72d
to
7fa1d3f
Compare
Hi @megan07 Is this PR ready to be merged? I don't know if you have any ETA for the merge, we would probably go with a custom build from master when this patch is merged. |
hi @jlpedrosa - thanks for following up on this PR! the changes still depend on an upgrade to the provider's |
Output of acceptance testing (with enterprise account):
|
Hi @jlpedrosa 👋, please find the changes now in |
Hi @anGie44 Thanks for the follow up! As soon as it is released we will incorporate it in out project. Will let you know if there's any feedback we can give. |
…sync_support addition of team sync support
"group": { | ||
Type: schema.TypeSet, | ||
Optional: true, | ||
ForceNew: true, |
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.
@anGie44 I know this has been long part of the provider, but what was the reasoning for this? At first sight, it doesn't seem like an update to the groups should require a new resource. Why wasn't in-place updating possible?
Addition of
team_sync_groups
data source andteam_sync_group_mapping
resource to address GitHub's new Team Sync capabilities as discussed in issue: https://github.com/terraform-providers/terraform-provider-github/issues/293