Update go-github to v29. #342
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
added
the
springerigor
reviewed
Feb 4, 2020
There was a problem hiding this comment.
Looks OK, only one concern about go.mod file.
In regard to the acceptance tests, did you read https://github.com/terraform-providers/terraform-provider-github#acceptance-test-prerequisites?
go.mod
Outdated
| @@ -1,9 +1,13 @@ | |||
| module github.com/terraform-providers/terraform-provider-github | |||
|
|
|||
| require ( | |||
| github.com/google/go-github/v28 v28.1.1 | |||
| github.com/google/go-github/v28 v28.1.1 // indirect | |||
There was a problem hiding this comment.
Not sure if v28 should still be there after the switch to v29.
There was a problem hiding this comment.
I haven't used go mod vendor before, but I can confirm that the string v28 doesn't appear anywhere in this repository except right in this file. Perhaps there is a transitive dependency on the old version?
There was a problem hiding this comment.
It may be true, you can check if go mod tidy will make a difference.
I sure did. And actually those docs seem to be out of date. In addition to the env vars outlined there, you also have to set $ cd terraform-provider-github
$ GITHUB_ORGANIZATION=github-test-jakebiesinger GITHUB_TEST_USER=jakebiesinger GITHUB_TEST_COLLABORATOR=jakebiesinger-onduo GITHUB_TEMPLATE_REPOSITORY=test-repo-template make testaccThe failure seems related to some server that isn't running on |
|
|
|
@jakebiesinger-onduo @springerigor any idea when this one might land? would love to finally close this one off in our environment 😄 |
I saw the same failures I'd seen previously related to something on |
|
👋 I've been having similar difficulties getting acceptance tests to work locally and have been trialling an acceptance test Action ahead of merging it here. In the meantime, here is the acceptance test run output: I have seen |
|
@jakebiesinger-onduo could you do another update to v29.0.3 to allow implementation of #271 ? Latest version includes google/go-github#1402 that is required to implement #271 and I think it's not worth starting without upgrading to v29 first. |
Merged
|
Hi, I have just raised #362 to implement GH-271 which requires this to be merged with the application of @martinssipenko's comment for upversioning to v29.0.3. |
|
I have #265 waiting for this to be merged, If this is at a standstill it would be great to get this merged as @benj-fletch suggested 🙌 |
|
I would like to spend a couple of days reviewing the acceptance test failures that this has introduced. It is my goal to 🚢 this one by end of week. Thanks for the sustained patience here! |
|
I've done a round of testing and have the following results:
The I have made a local commit that removes the This is still on track to 🚢 by end of week. I'd like another day to dig into the remaining regressions and am seeking a 👍 on deferring the |
|
That patch looks good to me. |
|
@jcudit I'm happy to update versions of either go or the github API, or to have someone else take this PR and run with it. I don't want to interrupt your investigation, so let me know how you'd like me to proceed. |
|
@jakebiesinger-onduo please rebase this branch onto @benj-fletch I am having trouble with root causing these remaining new failing cases. A fix to get the tests passing or any guidance on initial steps I could take would be appreciated! |
jakebiesinger-onduo
force-pushed
the
master
branch
from
d34232f to
87f35b0
Compare
- Removes newly addes uses of `v28` - Removes `go 1.13` upgrade
|
@jcudit I've rebased to latest master and applied your patch. PTAL. |
|
I've read through the changes we are pulling in here and do not see anything that would manifest in the test failures we're experiencing. While investigating these failing test cases locally I witnessed them pass occasionally. It seems like a race condition where a repository is deleted and then an invitation to collaborate on the repository is subsequently failed because the repository no longer exists. This gives me confidence in the changes this PR is introducing and would like to move forward with merging. |
|
I was in the process of working on #305 when I realized we need to upgrade to v29. Looking forward to this! |
kfcampbell
pushed a commit
to kfcampbell/terraform-provider-github
that referenced
this pull request
Jul 26, 2022
Update go-github to v29.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Updates to go-github v29. To do this, I ran
go mod vendorafter updatingvendor/modules.txt.There were two small interface changes I had to make manually:
client.Teams.EditTeamnow takes a third option,removeParentwhich I've set tofalseclient.Repositories.AddCollaboratornow returns a data object representing the invitation. This didn't seem to be used in the surrounding code so I am just ignoring it.I also ran acceptance tests locally. There are a few failures, but as a new contributor, I'm not sure how to proceed fixing them: