-
Notifications
You must be signed in to change notification settings - Fork 768
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
GitHub org ignore archived repos #1833
Conversation
`, randomID, testOrganization, testOrganization) | ||
|
||
check := resource.ComposeTestCheckFunc( | ||
resource.TestCheckOutput("should_be_false", "false"), |
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 uses terraform-plugin-sdk/v2
, while the GitHub provider uses v1
. 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
in data_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 wait
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.
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 of terraform-exec
, to a version above v0.16.0
, which deprecated the tfinstall
package that v1 of the sdk depends on
I just go stuck with the tests using the testAccProviders
(defined in github/provider_test.go
), which uses v1. They cannot be used for the v2 tests in github/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.
@kfcampbell with the completion of #1780 this now works locally, along with the test for 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.
I'm glad the SDK bump unblocked this work! Confirming that the added integration test is passing for me.
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [github](https://registry.terraform.io/providers/integrations/github) ([source](https://github.com/integrations/terraform-provider-github)) | required_provider | patch | `6.0.0` -> `6.0.1` | --- > [!WARNING] > Some dependencies could not be looked up. Check the Dependency Dashboard for more information. --- ### Release Notes <details> <summary>integrations/terraform-provider-github (github)</summary> ### [`v6.0.1`](https://github.com/integrations/terraform-provider-github/releases/tag/v6.0.1) [Compare Source](https://github.com/integrations/terraform-provider-github/compare/v6.0.0...v6.0.1) #### What's Changed - build(deps): bump github.com/golangci/golangci-lint from 1.56.1 to 1.56.2 by [@​dependabot](https://github.com/dependabot) in [https://github.com/integrations/terraform-provider-github/pull/2159](https://github.com/integrations/terraform-provider-github/pull/2159) - build(deps): bump github.com/hashicorp/terraform-plugin-sdk/v2 from 2.31.0 to 2.32.0 by [@​dependabot](https://github.com/dependabot) in [https://github.com/integrations/terraform-provider-github/pull/2160](https://github.com/integrations/terraform-provider-github/pull/2160) - build(deps): bump github.com/hashicorp/terraform-plugin-sdk/v2 from 2.32.0 to 2.33.0 by [@​dependabot](https://github.com/dependabot) in [https://github.com/integrations/terraform-provider-github/pull/2168](https://github.com/integrations/terraform-provider-github/pull/2168) - Fix github_external_groups page title by [@​tomasmota](https://github.com/tomasmota) in [https://github.com/integrations/terraform-provider-github/pull/2170](https://github.com/integrations/terraform-provider-github/pull/2170) - docs: Update example usage to use version 6.0 by [@​rnestler](https://github.com/rnestler) in [https://github.com/integrations/terraform-provider-github/pull/2169](https://github.com/integrations/terraform-provider-github/pull/2169) - fix: Make allowed_actions_config optional by [@​Danielku15](https://github.com/Danielku15) in [https://github.com/integrations/terraform-provider-github/pull/2114](https://github.com/integrations/terraform-provider-github/pull/2114) - GitHub org ignore archived repos by [@​felixlut](https://github.com/felixlut) in [https://github.com/integrations/terraform-provider-github/pull/1833](https://github.com/integrations/terraform-provider-github/pull/1833) - build(deps): bump github.com/stretchr/testify from 1.8.4 to 1.9.0 by [@​dependabot](https://github.com/dependabot) in [https://github.com/integrations/terraform-provider-github/pull/2177](https://github.com/integrations/terraform-provider-github/pull/2177) - build(deps): bump golang.org/x/crypto from 0.19.0 to 0.21.0 by [@​dependabot](https://github.com/dependabot) in [https://github.com/integrations/terraform-provider-github/pull/2180](https://github.com/integrations/terraform-provider-github/pull/2180) - build(deps): bump actions/add-to-project from 0.5.0 to 0.6.0 by [@​dependabot](https://github.com/dependabot) in [https://github.com/integrations/terraform-provider-github/pull/2175](https://github.com/integrations/terraform-provider-github/pull/2175) #### New Contributors - [@​tomasmota](https://github.com/tomasmota) made their first contribution in [https://github.com/integrations/terraform-provider-github/pull/2170](https://github.com/integrations/terraform-provider-github/pull/2170) - [@​rnestler](https://github.com/rnestler) made their first contribution in [https://github.com/integrations/terraform-provider-github/pull/2169](https://github.com/integrations/terraform-provider-github/pull/2169) - [@​Danielku15](https://github.com/Danielku15) made their first contribution in [https://github.com/integrations/terraform-provider-github/pull/2114](https://github.com/integrations/terraform-provider-github/pull/2114) **Full Changelog**: integrations/terraform-provider-github@v6.0.0...v6.1.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4yMjcuMCIsInVwZGF0ZWRJblZlciI6IjM3LjIyNy4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9--> Co-authored-by: kireque-bot[bot] <143391978+kireque-bot[bot]@users.noreply.github.com>
Resolves #1826
Before the change?
repositories
attribute of agithub_organization
always included all repos in the entire Organziation, including archived ones.After the change?
repositories
attribute of agithub_organization
can now selectively ignore archived repos for therepositories
attribute.Pull request checklist
Does this introduce a breaking change?
No
Please see our docs on breaking changes to help!