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

Upgrade go-github to v39 #905

Merged
merged 1 commit into from
Oct 5, 2021
Merged

Conversation

lauraseidler
Copy link
Contributor

This should allow moving forward with #649 (I'll provide another PR).

Release: https://github.com/google/go-github/releases/tag/v39.0.0 - the only breaking changes are in relation to ListRepositoryAccessRunnerGroup, which this provider isn't using as far as I can see.

Note: I'm not really a Go pro, so in case I messed something up, please let me know so I can learn :)

@jcudit jcudit added this to the v4.16.0 milestone Sep 21, 2021
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.

This looks reasonable to me!

@xun-guo-anzx
Copy link
Contributor

Hi @lauraseidler, with this change in release 4.15.0, I think ListRepositoryAccessRunnerGroup is referenced in resource_github_actions_runner_group. I assume this needs to be updated now?

@lauraseidler
Copy link
Contributor Author

Yeah, looks like it. Though I'm unsure what to pass there - I guess technically, this call should be looped to make sure to fetch all pages?

Or we pass the default from GitHub (30 per page, page 1), as that would keep the current behaviour as is, and then full pagination can be implemented separately?

Thoughts @xun-guo-anzx?

@xun-guo-anzx
Copy link
Contributor

@lauraseidler here is an example how we can handle pagination.

We can pass 30 per page as a start point, then fetch the rest of pages via a loop. Hope this can be useful for you.

@kfcampbell
Copy link
Member

@xun-guo-anzx what's the behavior if nil is passed for options to ListRepositoryAccessRunnerGroup?

@xun-guo-anzx
Copy link
Contributor

@xun-guo-anzx what's the behavior if nil is passed for options to ListRepositoryAccessRunnerGroup?

It should work the same as current behaviour, which only fetches the first 30 repository from the list

@lauraseidler
Copy link
Contributor Author

I've rebased this now and am passing nil to ListRepositoryAccessRunnerGroup here: https://github.com/integrations/terraform-provider-github/pull/905/files#diff-3a784b7896a69029f7895122c10d1858ade59409457828a7c3f4e0a17ae1207aR172

Personally, I would stick to this as the scope of this PR, as it should keep the behaviour exactly the same as before and only does the update, which should unblock both #649 and also implementing pagination for this in a follow up PR.

Does that work @xun-guo-anzx @kfcampbell?

@kfcampbell
Copy link
Member

Yep that sounds good to me!

@jcudit jcudit merged commit f9092a1 into integrations:main Oct 5, 2021
@lauraseidler lauraseidler deleted the go-github-v39 branch October 5, 2021 21:03
kfcampbell pushed a commit to kfcampbell/terraform-provider-github that referenced this pull request Jul 26, 2022
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.

4 participants