-
Notifications
You must be signed in to change notification settings - Fork 781
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
GitHub org ignore archived repos #1833
Merged
kfcampbell
merged 11 commits into
integrations:main
from
felixlut:github-org-ignore_archived_repos
Mar 4, 2024
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
577e1a0
add ignore_archived_repos argument to github_organization datasource
felixlut 886f53f
add test for checking that the archived repo is ignored
felixlut e11f5fb
add documentation for ignore_archived_repos
felixlut d59d8a9
Merge branch 'main' into github-org-ignore_archived_repos
felixlut 623bd63
Merge branch 'main' into github-org-ignore_archived_repos
felixlut c327730
Merge branch 'main' into github-org-ignore_archived_repos
felixlut 31416ea
Merge branch 'main' into github-org-ignore_archived_repos
felixlut ae201da
Merge remote-tracking branch 'origin/main' into github-org-ignore_arc…
c5655b7
fix ordering of imports
7f52d1b
Merge branch 'main' into github-org-ignore_archived_repos
kfcampbell 427938a
Merge branch 'main' into github-org-ignore_archived_repos
kfcampbell File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
Oops, something went wrong.
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.
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.
@kfcampbell this test is based on the ones I found here, where it appears that
TestCheckOutput()
handles the conversions of bools and strings, i.e.,false
-->"false"
. That code usesterraform-plugin-sdk/v2
, while the GitHub provider usesv1
. Is there a reason for not using v2 here?I think I read somewhere in the contribution docs that you guys don't want the developer community to mess with bumping dependencies ourselves, which is why I'm asking 😄
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.
The reason is due to lack of prioritization of #1780, not anything super intentional. I'm fine adding v2 as a dependency since we'll need to cut over to it anyways.
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.
So how do we move forward here? Should I wait until #1780 is merged, or import and use
v2
indata_source_github_organization_test.go
only (if that is even possible, haven't dealt much with golang dependencies and vendoring before)? I don't care about getting this in fast, so if it takes another couple of weeks for #1780, I'm fine to waitThere 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.
Let's move forward with your second option. The approach you describe is indeed possible and I think it's unlikely #1780 is going to be merged in the near future.
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.
I played around a bit with option 2, but I think using both v1 and v2 of the sdk is not compatible, at least not without doing some real weird stuff.
First of you need to use a version of the sdk before
v2.10.1
(which released in dec 2021). Using a later version requires a bump ofterraform-exec
, to a version abovev0.16.0
, which deprecated thetfinstall
package that v1 of the sdk depends onI just go stuck with the tests using the
testAccProviders
(defined ingithub/provider_test.go
), which uses v1. They cannot be used for the v2 tests ingithub/data_source_github_organization_test.go
.I looked into using the terraform-plugin-testing package as well, since that is the recommended solution for testing, but that seems to require v2 as well. I'm not spending anymore time investigating it, I'll wait for #1780. Unless you have some idea of how to test this feature that wont require a bump to v2?
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.
Oh gross...thanks for doing the legwork on that! I'm 👍 on waiting for now.
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.
If you're interested in picking up #1780 and fixing the build errors, please feel free! You should be able to create a new PR off of my branch if you desire.
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.
@kfcampbell that thought did cross my mind, especially since the time I spent working around it could have been enough to just make it work properly. I learnt a lot about go dependencies at least 😅
I may take a look later
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.
Haha, I'm glad you were able to take something positive out of it! Should you end up deciding to do so, I'd be happy to look at it.
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.
With the new release this actually works, I've tested it locally.