Skip to content
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

Add support for github repositories #13

Closed
wants to merge 30 commits into from

Conversation

Feggah
Copy link

@Feggah Feggah commented Jun 22, 2021

Description of your changes

This PR implements the GitHub repository. There are some fields that are overly verbose and they are commented as TODO: in the repository_types.go file. If you think that is necessary to represent them all in this pull request, please request changes.

Besides that, there is an error that I wasn't so sure if I should ignore it or not. When a repository is created without being initialized and then the user tries to instantly update the repository fields, a default value for defaultBranch is sent to the SDK, which is populated by the API when the repository was created. But the API returns an error saying: Cannot update default branch for an empty repository. Please init the repository and push first.

The problem is, if I ignore 422 errors (Validation failed), when a user actually tries to update the default branch of the repository, it will not show to them in the kubectl describe, so I decided to show this error to them, even if they experience this edge case.

How to reproduce this error:

Create a repository for your user using kubectl apply:

apiVersion: repositories.github.crossplane.io/v1alpha1
kind: Repository
metadata:
  name: sample
spec:
  forProvider:
    owner: <your-username>
  providerConfigRef:
    name: default

Check that the API returns the default_branch value, even if the repository it not initialized (this means that the RepositorySpec will be updated with this value):

https://api.github.com/repos/your-username/sample

Update this repository that was created before initializing it:

apiVersion: repositories.github.crossplane.io/v1alpha1
kind: Repository
metadata:
  name: sample
spec:
  forProvider:
    owner: <your-username>
    hasIssues: false
  providerConfigRef:
    name: default

Then, describe your repository to check the events:

kubectl describe repository sample

You should see that in the last event occurred an error CannotUpdateExternalResource, but the update was actually successful. The output should be something like:

Events:
  Type     Reason                        Age              From                                                  Message
  ----     ------                        ----             ----                                                  -------
  Normal   CreatedExternalResource       24s              managed/repository.repositories.github.crossplane.io  Successfully requested creation of external resource
  Warning  CannotUpdateExternalResource  1s (x2 over 1s)  managed/repository.repositories.github.crossplane.io  cannot update Repository: PATCH https://api.github.com/repos/feggah/sample: 422 Validation Failed [{Resource:Repository Field:default_branch Code:invalid Message:Cannot update default branch for an empty repository. Please init the repository and push first.}]

My conclusion is: there is times that this error is import to be shown to the user, especially when they try to make a default branch that is not yet pushed to the repository. If we decide to ignore it, we can't verify if the user is using the defaultBranch parameter improperly. To ensure that the user uses this parameter properly, I put in the documentation for this parameter:

// Name of the default branch
// The branch must already exist in the repository.
// +optional
DefaultBranch *string `json:"defaultBranch,omitempty"`

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

  • Created unit tests
  • Manually tested with the whole life-cycle of the resource (create, update and delete)

hasheddan and others added 29 commits June 15, 2021 10:27
Bumps crossplane-runtime to v0.13.0, which also updates a few other
dependencies.

Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
Updates to the more modern ProviderConfig. We drop support for Provider
since provider-github has never been released.

Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
Adds an example ProviderConfig resource.

Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
Uses the default crossplane-runtime rate limiter and adds a
ProviderConfig controller.

Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
Adds an organization membership type.

Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
Adds an organization membership controller. This controller takes a
naive approach to error handling and does not have unit tests, but is
being merged early to allow other folks to continue to make progress on
this provider.

Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
Updates org CRD to use correct github category instead of aws.

Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
Changing versions to accomodate CI.

Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
Signed-off-by: Feggah <gabidferreira9@gmail.com>
Signed-off-by: Feggah <gabidferreira9@gmail.com>
Signed-off-by: Feggah <gabidferreira9@gmail.com>
Signed-off-by: Feggah <gabidferreira9@gmail.com>
Signed-off-by: Feggah <gabidferreira9@gmail.com>
Signed-off-by: Feggah <gabidferreira9@gmail.com>
Signed-off-by: Feggah <gabidferreira9@gmail.com>
Signed-off-by: Feggah <gabidferreira9@gmail.com>
Signed-off-by: Feggah <gabidferreira9@gmail.com>
Signed-off-by: Feggah <gabidferreira9@gmail.com>
Signed-off-by: Feggah <gabidferreira9@gmail.com>
Signed-off-by: Feggah <gabidferreira9@gmail.com>
Signed-off-by: Feggah <gabidferreira9@gmail.com>
Signed-off-by: Feggah <gabidferreira9@gmail.com>
Signed-off-by: Feggah <gabidferreira9@gmail.com>
Signed-off-by: Feggah <gabidferreira9@gmail.com>
The external name of the repository is now set in the metadata name or annotations

Signed-off-by: Feggah <gabidferreira9@gmail.com>
@kzap
Copy link

kzap commented Nov 17, 2021

I ran this locally using go run ./cmd/provider/main.go --debug

Created this repo: https://github.com/mydev-org/my-service-github-provider

When I create a repository object and it already exists I get this panic
github.com/go-logr/zapr.(*zapLogger).Error
	/Users/amarcelo/go/pkg/mod/github.com/go-logr/zapr@v0.2.0/zapr.go:132
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler
	/Users/amarcelo/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.8.0/pkg/internal/controller/controller.go:297
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem
	/Users/amarcelo/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.8.0/pkg/internal/controller/controller.go:248
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func1.1
	/Users/amarcelo/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.8.0/pkg/internal/controller/controller.go:211
k8s.io/apimachinery/pkg/util/wait.JitterUntilWithContext.func1
	/Users/amarcelo/go/pkg/mod/k8s.io/apimachinery@v0.20.1/pkg/util/wait/wait.go:185
k8s.io/apimachinery/pkg/util/wait.BackoffUntil.func1
	/Users/amarcelo/go/pkg/mod/k8s.io/apimachinery@v0.20.1/pkg/util/wait/wait.go:155
k8s.io/apimachinery/pkg/util/wait.BackoffUntil
	/Users/amarcelo/go/pkg/mod/k8s.io/apimachinery@v0.20.1/pkg/util/wait/wait.go:156
k8s.io/apimachinery/pkg/util/wait.JitterUntil
	/Users/amarcelo/go/pkg/mod/k8s.io/apimachinery@v0.20.1/pkg/util/wait/wait.go:133
k8s.io/apimachinery/pkg/util/wait.JitterUntilWithContext
	/Users/amarcelo/go/pkg/mod/k8s.io/apimachinery@v0.20.1/pkg/util/wait/wait.go:185
k8s.io/apimachinery/pkg/util/wait.UntilWithContext
	/Users/amarcelo/go/pkg/mod/k8s.io/apimachinery@v0.20.1/pkg/util/wait/wait.go:99
Here's my object:
apiVersion: repositories.github.crossplane.io/v1alpha1
kind: Repository
metadata:
  name: my-service-github-provider
spec:
  forProvider:
    owner: mydev-org
    org: mydev-org
    description: "Test repo created using Crossplane GitHub Provider"
    homepage: https://mydev.org
    hasPages: false
    hasProjects: false
    hasWiki: false
    licenseTemplate: MIT
    gitignoreTemplate: Terraform
    deleteBranchOnMerge: true

  providerConfigRef:
    name: default
  • Also I notice upon repo creation, there's always an error right after
Events:
  Type     Reason                        Age                    From                                                  Message
  ----     ------                        ----                   ----                                                  -------
  Warning  CannotCreateExternalResource  7m22s (x8 over 9m20s)  managed/repository.repositories.github.crossplane.io  cannot create Repository: POST https://api.github.com/orgs/mydev-org/repos: 422 Repository creation failed. [{Resource:Repository Field:name Code:custom Message:name already exists on this account}]
  Normal   CreatedExternalResource       9s                     managed/repository.repositories.github.crossplane.io  Successfully requested creation of external resource
  Warning  CannotCreateExternalResource  8s (x3 over 9s)        managed/repository.repositories.github.crossplane.io  cannot create Repository: POST https://api.github.com/orgs/mydev-org/repos: 422 Repository creation failed. [{Resource:Repository Field:name Code:custom Message:name already exists on this account}]
  • I cant update the repo anymore after creating it because of the 422 error

@kzap
Copy link

kzap commented Nov 17, 2021

Also wondering about why the metadata.name is used as the repository name, is this a crossplane convention, rather than setting spec.name or something?

I can imagine its hard to rename an object by deploying a new k8s object with a new metadata.name? Is that related to the external-name discussion crossplane/crossplane#2532 ?

Signed-off-by: Feggah <gabidferreira9@gmail.com>
@Feggah
Copy link
Author

Feggah commented Nov 18, 2021

Hey @kzap , sorry for my delayed response! Thanks for testing this PR! I'm happy to know that it would be useful to you 🙂

I tried to reproduce your errors and I didn't get any of them. Maybe our environments differ in versions and it is causing those problems?

One thing that I want to point out is that I needed to change the generation version of CRDs to apiextensions.k8s.io/v1 instead of v1beta1, because it is no longer supported in my local version of kubectl. Maybe you could test if this change fixed your errors?

Client Version: version.Info{Major:"1", Minor:"22", GitVersion:"v1.22.1", GitCommit:"632ed300f2c34f6d6d15ca4cef3d3c7073412212", GitTreeState:"clean", BuildDate:"2021-08-19T15:45:37Z", GoVersion:"go1.16.7", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"22", GitVersion:"v1.22.1", GitCommit:"632ed300f2c34f6d6d15ca4cef3d3c7073412212", GitTreeState:"clean", BuildDate:"2021-08-19T15:39:34Z", GoVersion:"go1.16.7", Compiler:"gc", Platform:"linux/amd64"}

If you are still getting those errors after this, send a message here again and I will try to help you!

Also wondering about why the metadata.name is used as the repository name, is this a crossplane convention, rather than setting spec.name or something?

Yes, you got it right. It's a Crossplane convention. We use metadata.name or annotations.crossplane.io/external-name to set the name of managed resources in the external providers. Check it out this documentation for further explanation.

I can imagine its hard to rename an object by deploying a new k8s object with a new metadata.name? Is that related to the external-name discussion crossplane/crossplane#2532 ?

Yes.. that's a tricky discussion about renaming external names/identifiers for the resources. I actually support this in this PR, you can see that I duplicated the name in the Status struct to keep track of the "last seen name" and then I try to GET it twice before stating that the resource doesn't exists. But this solution isn't well accepted and probably needs more discussion to define which way we should go to solve this. Maybe you can help with some inputs in the issue that you referenced?

@Feggah
Copy link
Author

Feggah commented Dec 2, 2023

Self-rejecting the PR based on no interest of the community and also no bandwidth to support it if any request for changes appears.

If anyone needs it, feel free to use the code here as a starting point and open another PR 👍🏼

@Feggah Feggah closed this Dec 2, 2023
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants