-
Notifications
You must be signed in to change notification settings - Fork 770
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
provider: Add the possibility to create repositories for non-organization accounts #96
provider: Add the possibility to create repositories for non-organization accounts #96
Conversation
@justincampbell Hi, I wrote this change in the way that the organization management is not disrupted. I also wanted to test this on the deep level, but unfortunately I don't belong to any orgs on github. I asked github support to give me free/trial; ~2days organization account for that purpose, but they told me that it's not possible and that you can add me to the terraform organization for free as a contributor. |
You can create organizations for free on Github. I have a few. Paid account is not needed for this. |
Thank you man. I didn't know that - perhaps github support should also know that. |
@eliaszs see https://help.github.com/articles/converting-a-user-into-an-organization/ for how to do it |
Thanks @lkysow. I have seen that I just didn't have time to get back to this PR as other job took over. I will make some updates shortly. |
Unfortunately changing the name in the schema is a breaking change, we should deprecate and provide a migration path for people so their configs won't just stop working. AWS and other providers have a lot of examples of migrations and how to do this: https://github.com/terraform-providers/terraform-provider-aws/blob/b8d4e1570fc43a2acee6b6e47f63c9db6b067fa2/aws/resource_aws_codebuild_project_migrate.go The simpler path may be just to keep using the |
6f15d5c
to
91fc54b
Compare
The PR can be merged to master ✅There are no breaking changes anymore 😁 @paultyng The schema hasn't been changed for any of the resources implemented by the project. The only change was made to the provider's config argument - I restored the GITHUB_ORGANIZATION variables so that people won't need to update their configs to handle the new version. 👍 I wondered wether below configuration wouldn't be more flexible in terms of managing github resources in terraform. 🤔 provider "github" {
"alias" = "admin"
"token" = "${var.github_token}"
}
provider "github" {
"alias" = "user"
"token" = "${var.github_token}"
}
resource "github_organization" "google" {
provider = "github.admin"
}
resource "github_organization" "amazon" {
provider = "github.admin"
}
resource "github_repository" "alexa" {
provider = "github.user"
owner = "${github_organization.google.name}"
}
resource "github_repository" "alexa" {
provider = "github.user"
owner = "${github_organization.amazon.name}"
} |
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.
Hi @eliaszs
thank you for making those changes and sorry for the delay.
I took another look at your PR and here's some notes I'd like to get through:
As @paultyng mentioned we should deprecate the existing organization
field. It's good that it's back in the schema, but we aren't communicating the deprecation to the user. The best way to do this would be adding Deprecated: "Use owner field (or GITHUB_OWNER ENV variable)",
to the field.
We should probably just replace GITHUB_ORGANIZATION
with GITHUB_OWNER
in the acceptance tests.
I rebased your branch and resolved all the conflicts with current master
(see https://github.com/terraform-providers/terraform-provider-github/compare/eliaszs/feature/individual_account and feel free to use it as a base for your next changes) so I can run all acceptance tests.
I ran these tests first with our test org (terraformtesting
) to verify that everything still works the same way it did with org-based owner - we're good on that front ✅ .
Then I ran them again with user owner (terraformbot
), which resulted in many failures we'll need to address. A few suggestions related to that:
- To make things generally easier (and more predictable) I'm thinking we could discover the authenticated username and just pre-fill it for all API calls (if it's empty), instead of putting any conditionals down to resources.
- While many resources are going to be useful in the user-owner-context (e.g. most
github_repository_*
), some just won't (e.g.github_membership
,github_organization_webhook
,github_team_*
etc.).- We should check whether the
owner
is org and if not, return a user-friendly error, e.g."This resource can only be used in the context of an organization, %q is a user"
- Likewise, we should conditionally skip relevant acceptance tests, if these run with
GITHUB_OWNER
that is not an org.
- We should check whether the
Generally speaking I'd like to see all acceptance tests passing (with the irrelevant ones being skipped, obviously) for GITHUB_OWNER=user-account
before moving on.
Let me know what you think.
name string | ||
client *github.Client | ||
StopContext context.Context | ||
Organization bool |
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.
How do you feel about renaming this to IsOrganization
to signal that it's a boolean?
} | ||
|
||
func (o *Owner) IsOrganization() bool { | ||
return o.Organization |
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.
Is there any particular benefit to having this function, when we can just access the exported attribute?
This would be nice to have for sure |
Very excited to see this PR! What's still needed to get it merged? |
Thanks for your efforts so far! Does anyone have plans to finish this and get it merged? I’d really love to see this provider work with personal repos! |
Any updates here? I, too, find it quite frustrating that I can't use this provider on personal repos. |
Any updates here? Do you need help for fixing this bug. |
I rebased the PR to #322. Even if tests have passed, I haven't tested if it works. Please report your findings there. |
@elislusarczyk @abitrolly hey all! Just wanted to see what the status is here and if anyone has been working on this branch recently since adding support for personal repos still seems like a recurring request. I'm happy to push up changes to handle the merge conflicts if needed :) |
@anGie44 I just rebased the changes in this PR to #322. More conflicts there since then. If I remember it right, the problem with this provider was that concepts of access credentials and repository ownership were mixed. It probably doesn't worth to invest more time into this for me, because I switched to GitLab for my automation since then, and it works. ) |
thanks @abitrolly! I was able to locally get the branch in a state w/o merge conflicts but I'm wondering if this work to tackle https://github.com/terraform-providers/terraform-provider-github/issues/45 is still in the pipeline (since this PR has gotten a bit stale over time) or is the graphql work being prioritized atm @jcudit ? |
@anGie44 I would like to merge an incremental change towards GraphQL support which would likely introduce conflicts here. Immediately afterwards, I think this feature should be prioritized over any others. The two changes would form Thank you for contributing on this one. Excited to see it merge. |
I see GraphQL as just a different way to access GitHub API, while fix for #45 requires fixing leaking abstraction in provider resources. What would be useful is to release |
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.
hi @elislusarczyk ! thanks again for this PR :) looks like it's gathered quite a bit of interest by the community. would you mind resolving the merge conflicts, updating where needed based on the last review? I've also created this PR https://github.com/terraform-providers/terraform-provider-github/pull/428/files off your work here to experiment w/the latest changes in master that you could use
I've pushed this one back a release as the next release has a similar focus around making the organization / non-organization split more approachable. |
sounds good @jcudit, thanks for the heads-up! question about continuing the implementation in #428: I've found myself wanting to add this support for non-organizations in other resources (like suggested in the tagged issues) but am realizing this will become an even bigger change to review. would it make sense to focus on on the repository resource in one PR and split out subsequent changes in separate PRs? |
Yes, please. I’ll do my best to review in a timely manner to keep momentum of the incremental changes. |
Closing in favour of https://github.com/terraform-providers/terraform-provider-github/pull/464. Thanks to everyone who contributed on this PR as it was valuable towards the overall goal of supporting non-organization accounts. |
Fixed the ambiguity regarding the owner parameter name when calling the go-github api and added the possibility for the individual accounts to create repositories.
Here is the 404 error when an individual account tries to create a repository.
Github REST Api resource
/orgs/eliaszs/repos
doesn't exist for user's account.The PR is only breaking the compatibility with the env variable name. The
organization
structure has been renamed toowner
together with the GITHUB_ORGANIZATION -> GITHUB_OWNER. It is now compatible with the go-github api in the most cases.According to the spec of this function the
org string
argument is really an owner and not the organization, as it is used in other function across go-github library.Here is the ambiguous case for the
Create
and below is the correct one for theGet
function in the go-github library.Fixes #45