Add repository file resource #318
Conversation
ghost
added
size/XL
Type: Documentation
|
For some additional context regarding the "Fix go deps" commit: when using go 1.13 I received the following: I resolved this by following the steps I found at https://tip.golang.org/doc/go1.13#version-validation, if it's preferred to move this commit/change to a separate PR I'm happy to do so. |
|
That change to |
| Delete: resourceGithubRepositoryFileDelete, | ||
| Importer: &schema.ResourceImporter{ | ||
| State: func(d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) { | ||
| parts := strings.Split(d.Id(), ":") |
There was a problem hiding this comment.
The parseTwoPartID function can be used for this.
There was a problem hiding this comment.
I considered parseTwoPartID until I saw that the error returned is quite specific and doesn't apply to what I'm parsing. As parseTwoPartID is used in a couple of other funcs I decided if I was going to refactor the returned error it probably needs it's own PR/discussion.
There was a problem hiding this comment.
I've got a PR at #313 to address that problem too, so if you switch to it now when that is merged your code will have to be updated to provide the proper labels for the left/right halves of the ID.
No problem, I've dropped that commit 👍 |
There was a problem hiding this comment.
@shoekstra thanks for this contribution. It lines up fairly well with functionality I was asking for over in this issue.
Pull Requests are a missing piece that I'd like to see added onto this either in a subsequent release or via a refactoring of this PR. Can you take a look at the linked issue and let me know if what was planned there aligns with your use cases that this patch solves? I am interested in your take on what next steps could be to involve the Pull Request functionality of the go-github client library.
Aside from that concern, here are a few questions I have around the implementation as-is:
-
I see the
client.Repositories.CreateFilefunction was used inresourceGithubRepositoryFileUpdateinstead of the availableclient.Repositories.UpdateFilefunction. What is the tradeoff around using one over the other? -
I see that a branch needs to exist before it can receive commits. What are your thoughts around adding support for creating a branch if it does not exist (and throwing an error if a branch already exists)? Are you currently automating the creation of branches through some other means?
-
Are you open to generalizing the resource from
_fileto_content? This would allow for declaring multiple files within a resource block and iterating through the lifecycle management of each one. Have there been any pain points with using multiplerepository_fileresources with your use cases so far?
|
Hey @jcudit,
Skipping the pull request bit was a conscious decision I made; I wanted a way to manage a few files (GitHub Actions in my case) in a bunch of repos from a central Terraform repo. The review would already happen when updating the Terraform plan and I decided creating additional PRs created additional overhead for someone to review the changes a second time and, more importantly, this didn’t fit with Terraform’s job of ensuring “desired state” in that a file would be described in a plan with desired content but this could not be ensured unless committing and pushing to the specified branch. Reading the issue you linked, the initial proposal is the better way to go should you want to add support for pull requests. With Terraform you want to manage the file and update it using a pull request, you don’t want to manage a pull request as a merged pull request would be void if the content is modified (I hope that makes sense). I'm not convinced managing pull requests is something to do but am open to more discussion on how we could make it work if it's functionality you're after.
Looking now I see they’re the same, there is no difference between them. At the time I referred to the API docs and saw creating and updating files was done using the same method, so I didn’t look for an Update function in
My use case is to create files in branches that already exist thus creating a branch is out of scope of this resource. If I wanted a branch to exist I would want to manage it via a
Sure, I'm happy to add a
I haven’t had any issues with multiple resources yet; at the moment I’ve declared 3 files in a module and reference that module 6 times in a plan, so effectively managing 18 files across 6 repos in a single Terraform run. |
👍 this did not occur to me and removes my desire to have Terraform initiate PRs. Thank you for addressing all other concerns raised as well. I think the workflow you are presenting here is valuable to the community and will work to get this merged and released as-is. My next steps are to:
|
|
@shoekstra I've merged #313. Can you rebase and consume the new I will look into adding the new branch and environment variables to CI in the meantime. |
Signed-off-by: Stephen Hoekstra <[email protected]>
|
I've rebased my branch. I was thinking about your requests re UX. It's out of scope of me at the moment, but there should be enough of a base for someone to pick up adding the Feel free to merge if you agree! |
…e_resource Add repository file resource
Hello,
This PR adds support to manage files in a git repo using the contents API. I think I've got most cases covered in tests and have updated the docs and README appropriately.
Note that certain CI tests will be skipped until the new
GITHUB_TEST_USER_NAMEandGITHUB_TEST_USER_EMAILvariables are exported.Stephen