Bump go-gitub to v60

[nayuta]

trafficstars

Resolves #2187

---

Before the change?

• go-github v57.0.0

After the change?

• go-github v60.0.0
  • Fix code for
    • https://github.com/google/go-github/pull/3070
    • https://github.com/google/go-github/pull/3073

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)

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

• [ ] Yes
• [x] No

---

[kfcampbell]

I've resolved the merge conflict, though I'm a little hesitant about the HookConfig changes. The strongly-typed HookConfig struct means we can no longer change 1 to true in the insecureSslStringToBool function. We could remove that line entirely, though that means https://github.com/integrations/terraform-provider-github/pull/2196 would essentially be undone. @EttoreFoti do you have thoughts on this change?

Our docs are pretty vague about this: insecure_ssl says "string or number" next to it. Presumably the string is meant to represent a boolean there, though the docs text mentions only 0 or 1 as valid options.

[EttoreFoti]

I've resolved the merge conflict, though I'm a little hesitant about the HookConfig changes. The strongly-typed HookConfig struct means we can no longer change 1 to true in the insecureSslStringToBool function. We could remove that line entirely, though that means #2196 would essentially be undone. @EttoreFoti do you have thoughts on this change?

Our docs are pretty vague about this: insecure_ssl says "string or number" next to it. Presumably the string is meant to represent a boolean there, though the docs text mentions only 0 or 1 as valid options.

@kfcampbell this is bumping 3 major version of the GitHub client which is core in the provider, all test should be rerun before approving, I'm testing on the issue that was fixed but is unfixed by this PR, I'll create new PR with fixed once done.

[connor-miller-kr]

Any update on this PR?

[Simon-Boyer]

@kfcampbell what is missing for this PR to go forward? Is there anything I can do to help? I have a project that would need to define rulesets with custom properties and we need go-github v60 to implement this properly in the provider. @nayuta If you dont have time to work on this right now, could you give me permission to write to your fork so I can give a hand?

[EttoreFoti]

@kfcampbell what is missing for this PR to go forward? Is there anything I can do to help? I have a project that would need to define rulesets with custom properties and we need go-github v60 to implement this properly in the provider. @nayuta If you dont have time to work on this right now, could you give me permission to write to your fork so I can give a hand?

@Simon-Boyer this is breaking a bunch of resources, being an update of the main SDK the full test suite needs to be run to ensure nothing breaks

[nayuta]

@nayuta If you dont have time to work on this right now, could you give me permission to write to your fork so I can give a hand?

@Simon-Boyer I quickly fixed compile errors and invite you to the repo.

[Simon-Boyer]

@kfcampbell @EttoreFoti Trying to make all tests pass, but I realized a lot of them are also not passing on main (10-15% of the tests are failing). Is this on my side? And if not, should I work to repair the tests that were passing and are no longer passing or do i really need to make all tests pass; that will require a substantial amount of work and I'm not sure i will be able to provide that.

[siddharthab]

If we are making the effort to upgrade, might as well upgrade to v62 now. It may resolve #2192 as well. Let me know if I can help.

[EttoreFoti]

@kfcampbell @EttoreFoti Trying to make all tests pass, but I realized a lot of them are also not passing on main (10-15% of the tests are failing). Is this on my side? And if not, should I work to repair the tests that were passing and are no longer passing or do i really need to make all tests pass; that will require a substantial amount of work and I'm not sure i will be able to provide that.

@Simon-Boyer some tests are broken since before, are you working on this branch/fork too? I have some time now so we can make an effort and do the job to cleanup the tests and bump straight to v62 as @siddharthab is saying. We can coordinate the effort to do so, let me know if you want to do it together or I'll get on it and try to close it.

[Simon-Boyer]

@EttoreFoti i was working on this branch yes. I dont have any time to put towards this this week, but I would be happy to help after. Just let me know some tasks i can tackle and I'll give a hand.

Also, do you want to cleanup the tests here? Or make a PR just for that separetly?

[kfcampbell]

Thank you all for the attention to testing! Our suite is not in a healthy place, and I really appreciate any attention given to make it better than it was.

I definitely support upgrading to v62 over v60 here.

I'd like to add again that my main concern is the HookConfig being strongly typed may make this workaround impossible, and cause a reversion that breaks the organization webhook resource, so that will need to be tested before merge.

[EttoreFoti]

@kfcampbell @Simon-Boyer I did the work, it was easier for me to start clean from main and bump myself to v62, I think this PR can be closed in favor of this one. I also fixed most of the broken tests, so now most of the resources are covered, the test suite would need a lot of love and an entire different activity though.

[Simon-Boyer]

@kfcampbell @Simon-Boyer I did the work, it was easier for me to start clean from main and bump myself to v62, I think this PR can be closed in favor of this one. I also fixed most of the broken tests, so now most of the resources are covered, the test suite would need a lot of love and an entire different activity though.

This is amazing! Thanks a lot for all the work!

[kfcampbell]

Closing as #2304 has been merged!