-
Notifications
You must be signed in to change notification settings - Fork 765
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
resource/repository: add create from template #309
Conversation
|
||
template { | ||
# FIXME: Change this to something more suitable for CI runs | ||
owner = "jcudit" |
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.
@paultyng - If there is a better repository or organization to use, please let me know.
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.
We generally try to avoid hard-coding organization or user names into the test suite, because that prevents anyone outside of the current team of maintainers from running these tests as they wouldn't have access to that organization or user.
We can leverage environment variables:
https://github.com/terraform-providers/terraform-provider-github/blob/2c03b951bb0aadc56d5f3e9a254f464d5ee018b8/github/provider_test.go#L58-L66
In this case I suppose we could leverage GITHUB_ORGANIZATION
and introduce a new variable, say GITHUB_TEMPLATE_REPOSITORY
?
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.
Thanks for the PR, I left you some comments inline. Feel free to request me directly for reviews in the future.
|
||
template { | ||
# FIXME: Change this to something more suitable for CI runs | ||
owner = "jcudit" |
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.
We generally try to avoid hard-coding organization or user names into the test suite, because that prevents anyone outside of the current team of maintainers from running these tests as they wouldn't have access to that organization or user.
We can leverage environment variables:
https://github.com/terraform-providers/terraform-provider-github/blob/2c03b951bb0aadc56d5f3e9a254f464d5ee018b8/github/provider_test.go#L58-L66
In this case I suppose we could leverage GITHUB_ORGANIZATION
and introduce a new variable, say GITHUB_TEMPLATE_REPOSITORY
?
@radeksimko thanks for the wisdom so far. I've pushed a commit to address your feedback but am still working through build errors. Following up on your suggestion around environment variable use:
EDIT: After sleeping on it, it occurred to me that the acceptance tests are not run in CI. I will verify the change locally instead and push a couple of other documentation and logging edits before re-requesting review. Let me know if I’ve gotten this wrong. |
Really interested to see this PR get merged. |
We run acceptance tests for most providers (including GitHub) in TeamCity currently. Apologies if you were not briefed about this as part of our on-boarding process! 🙈 I sent you some details via Slack as the instance isn't available for public, although we do have some plans in that area (making acceptance testing more accessible and easier for all providers). btw. We all internally use https://github.com/terraformtesting for these purposes - I also added you there as member, so you don't need to create any new org just to run these tests locally. |
* Create a new resource if `template` changes * Make required fields non-optional * Remove unnecessary length check * Defer bump to go 1.13 * Replace hard-coded test values with env vars
c5af1a6
to
8d098c1
Compare
The associated acceptance test is now passing alongside the unit tests for this change. I will re-request review at this time. |
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.
LGTM
* Create two topics within the repo named `test-topic` and `second-test-topic` | ||
* In the repo settings, make sure all features and merge button options are enabled. | ||
* `test-repo-template` | ||
* Configure the repository to be a [Template repository](https://help.github.com/en/github/creating-cloning-and-archiving-repositories/creating-a-template-repository) |
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.
Thank you so much for updating these instructions! ❤️
Update CHANGELOG.md to include #309
…-repos resource/repository: add create from template
…g-2.3.0 Update CHANGELOG.md to include integrations#309
Overview
This PR adds the ability to create a repository using a repository template.
Fixes https://github.com/terraform-providers/terraform-provider-github/issues/254.
NOTE: There is an outstanding
FIXME
that I've called out inline.Progress
website/docs/r/repository.html.markdown
template
configuration blockCreateFromTemplate
whentemplate
configuration is presentisTemplate
istrue