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

CRD GitHub repository secrets #7

Merged
merged 9 commits into from
Aug 26, 2021
Merged

CRD GitHub repository secrets #7

merged 9 commits into from
Aug 26, 2021

Conversation

Giomaster
Copy link

@Giomaster Giomaster commented Aug 19, 2021

Description of your changes

Fixes #

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

@Giomaster
Copy link
Author

Giomaster commented Aug 19, 2021

I try to run make reviewable test but i got some errors about some packages (Please, see details below to understand better the issue). Any help must be appreciated 😀

@Quitanias Quitanias requested review from Quitanias and removed request for Quitanias August 20, 2021 11:47
pkg/clients/secrets/secrets.go Outdated Show resolved Hide resolved
pkg/clients/secrets/secrets.go Outdated Show resolved Hide resolved
pkg/clients/secrets/secrets.go Outdated Show resolved Hide resolved
Copy link

@Feggah Feggah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I think we shouldn't change the folders structure for fake. Using provider-aws as a model, they use the fake folder inside each resource. We can follow this same pattern.

  • There is a problem with this managed resource name. We can't have two resources with the same name: the Kubernetes built-in secret and the repository secret.
    If we try to use kubectl get secret, it won't return the repository secrets. We need to change it, so we can easily get/describe repository secrets without conflicting with K8S secrets. I was thinking in something like githubsecret or ghsecret, any thoughts?

  • Last but not least, you need to sign off all your commits. Please, read the crossplane contribution process.

apis/secrets/v1alpha1/secrets_types.go Outdated Show resolved Hide resolved
apis/secrets/v1alpha1/secrets_types.go Outdated Show resolved Hide resolved
apis/secrets/v1alpha1/secrets_types.go Outdated Show resolved Hide resolved
pkg/clients/secrets/secrets.go Outdated Show resolved Hide resolved
pkg/clients/secrets/secrets.go Outdated Show resolved Hide resolved
pkg/controller/secrets/secrets.go Outdated Show resolved Hide resolved
pkg/controller/secrets/secrets.go Outdated Show resolved Hide resolved
pkg/controller/secrets/secrets.go Outdated Show resolved Hide resolved
pkg/controller/secrets/secrets.go Outdated Show resolved Hide resolved
pkg/controller/secrets/secrets.go Outdated Show resolved Hide resolved
@Giomaster Giomaster force-pushed the feat/reposecrets branch 2 times, most recently from be0641e to 4f7d303 Compare August 24, 2021 14:50
@Giomaster Giomaster requested a review from Feggah August 24, 2021 14:51
@Giomaster Giomaster force-pushed the feat/reposecrets branch 2 times, most recently from b82dd00 to 8d3c884 Compare August 25, 2021 18:38
Copy link

@Feggah Feggah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking the GitHub API, the Repository Secret is in Actions group. IMHO the folder structure should be actions/<...>. For example:

  • apis/actions/v1alpha1/repositorysecret_types.go
  • examples/actions/repositorysecret.yaml
  • pkg/clients/actions/repositorysecret.go
  • pkg/controller/actions/repositorysecret.go

examples/secrets/secrets.yaml Outdated Show resolved Hide resolved
pkg/clients/repositorysecrets/repositorysecrets.go Outdated Show resolved Hide resolved
pkg/controller/repositorysecrets/repositorysecrets.go Outdated Show resolved Hide resolved
@Feggah Feggah changed the base branch from main to develop August 25, 2021 20:48
@Giomaster Giomaster requested a review from Feggah August 25, 2021 22:14
Copy link

@Feggah Feggah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just two more nitpicks.

I have misinterpreted the behavior of the SDK in the method CreateOrUpdate, sorry for my reviews about it :)

examples/actions/secrets.yaml Outdated Show resolved Hide resolved
pkg/controller/actions/repositorysecrets.go Outdated Show resolved Hide resolved
@Giomaster Giomaster requested a review from Feggah August 26, 2021 13:45
Copy link

@Feggah Feggah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good @Giomaster ! Just added one more nitpick. After this, I will merge the PR.

Thanks! 🚀

apis/actions/v1alpha1/repositorysecrets_types.go Outdated Show resolved Hide resolved
Copy link

@Feggah Feggah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for my last approval, we may need to modify the code again. I was thinking in one use case and the CRD will not behave as expected.

If the user wants to "import" a secret (I know we can't import it, just create/update), this means that the CRD will update its value to which is defined in the K8S secret reference. But the problem is: the actual request will be an UPDATE (because the Repository Secret already exists in GitHub), but if you describe the resource, it will log that the resource have been CREATED (we expected it to be UPDATED).

This inconsistency is just a detail, but we must log properly to help investigating future issues.

To fix it, we may need to change this code:
https://github.com/stone-payments/provider-github/blob/2679fce4f836ac2c91889ced5d0dcac798582bd5/pkg/controller/actions/repositorysecrets.go#L101-L104
To a GET request, then, the IsUpToDate will return false (because we will compare hashes from the K8S Secret and Status -- which will be empty).

With this false response from IsUpToDate, crossplane-runtime will call Update() instead of Create().

If the GET request doesn't find any secrets, thats because we need to create the secret.

Fixing this, the logs behavior will be fixed :)

The code will be something like this:

sec, _, err := gh.GetRepoSecret(ctx, cr.Spec.ForProvider.Owner, cr.Spec.ForProvider.Repository, meta.GetExternal(name))
if err != nil {
	return managed.ExternalObservation{}, errors.Wrap(err, "cannot get Secret")
}

You still can ignore the error returned by the GetRepoSecret if the response status code is 404, which just means that we need to create the resource.

How to reproduce this issue

  1. Create a secret in any repository in GitHub using the interface.
  2. Apply the RepositorySecret manifest in Kubernetes with the same external-name as you defined in the step above.
  3. kubectl describe the RepositorySecret.

You will see that the log will contain CreatedExternalResource, but it should be UpdatedExternalResource

Signed-off-by: giomaster <giomaster127@gmail.com>

removing unecessary pkg in go.mod

Signed-off-by: giomaster <giomaster127@gmail.com>
Signed-off-by: giomaster <giomaster127@gmail.com>

add tests to CRD secrets and fix general scaffold

Signed-off-by: giomaster <giomaster127@gmail.com>
Signed-off-by: giomaster <giomaster127@gmail.com>

split mock path

Signed-off-by: giomaster <giomaster127@gmail.com>
Signed-off-by: giomaster <giomaster127@gmail.com>
Signed-off-by: giomaster <giomaster127@gmail.com>
Signed-off-by: giomaster <giomaster127@gmail.com>
Signed-off-by: giomaster <giomaster127@gmail.com>
Signed-off-by: giomaster <giomaster127@gmail.com>
Signed-off-by: giomaster <giomaster127@gmail.com>
Copy link

@Feggah Feggah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Great work :)

@Feggah Feggah merged commit df145a6 into develop Aug 26, 2021
@Feggah Feggah deleted the feat/reposecrets branch August 26, 2021 20:15
@Feggah Feggah restored the feat/reposecrets branch September 27, 2021 22:44
@Feggah Feggah mentioned this pull request Oct 14, 2021
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants