search

integrations / terraform-provider-github

Terraform GitHub provider
https://www.terraform.io/docs/providers/github/
MIT License
879 stars 719 forks source link

Bump go-gitub to v60 #2188

Closed nayuta closed 1 month ago

nayuta commented 5 months ago

Resolves #2187

Before the change?

After the change?

Pull request checklist

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

kfcampbell commented 4 months ago

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 commented 4 months ago

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 commented 3 months ago

Any update on this PR?

Simon-Boyer commented 3 months ago

@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 commented 3 months ago

@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 commented 3 months ago

@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 commented 2 months ago

@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 commented 2 months ago

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 commented 2 months ago

@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 commented 2 months ago

@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 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

@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 commented 1 month ago

Closing as #2304 has been merged!