My changes have reasonable test coverage

The unit tests pass, but I don't have environments/credentails for e2e tests.

I didn't add a unit test because there wasn't a clean obvious place to add a test. A pointer is appreciated.

I ensured my PR is ready for review with make reviewable

I'm unable to run the docs target (here or on main), there are also a number of linter warnings that also exist on main.

hi @scothis ! Do you have any documentation for Service Binding project?

@gusfcarvalho yes, we do. There are both a human friendly overview of the provisioned service and the formal spec.

There is also a reference implementation of the spec.

I'm unable to run the docs target (here or on main), there are also a number of linter warnings that also exist on main.

We're currently running our CI with golangci-lint 1.49.0 and golang 1.19. If you use them you should be able to run make reviewable.

I would like to discuss this PR in a community meeting, if we can - more on the 'integrating with non-vanilla k8s projects', as this is something that we didn't do until now (like e.g. integrating with bitnami/sealedsecrets).

Having said that, I really see no issue with a single status field (although from the spec it looks like we would need to add more things? 🤔)

In any case we can make sure the PR is ready to be merged when we get to the community meeting to discuss it.

cc @moolen @sebagomez @knelasevero

Having said that, I really see no issue with a single status field (although from the spec it looks like we would need to add more things? 🤔)

That seems ok at first glance but does not seem sustainable in the long run, regarding having to review new similar inclusions and to document every new inclusion so people know the integration exists... It is somewhat "worse" than our provider situation because at least providers are adding new self-contained and explicit functionalities to the project. In this case, it feels a bit off.

Maybe some status fields and how they are updated/or how they work could also be abstracted away, pluginazable and offered as extension points?

Agree with discussing at next community meeting though +1

I would like to discuss this PR in a community meeting, if we can - more on the 'integrating with non-vanilla k8s projects', as this is something that we didn't do until now (like e.g. integrating with bitnami/sealedsecrets).

👍

from the spec it looks like we would need to add more things? 🤔

This PR covers everything required by the spec for a provisioned service. There are a couple recommendations that are left unaddressed. They are:

• Adding a label to the externalsecrets.external-secrets.io CRD. This is intended to help tools discovered compatible resources, but I'm not aware of anything that actually uses it.
• Create secrets with certain fields. The exact shape of the secret is left up to the service/user to define. The .data.type field in the Secret is required when mounted in the container file system, but is not required at this point as it can be defined by the user's ServiceBinding resource.

We're currently running our CI with golangci-lint 1.49.0 and golang 1.19. If you use them you should be able to run make reviewable.

I'm still hitting an error with the docs target

make docs

10:16:05 [ OK ] Finished generating deepcopy and crds
/Library/Developer/CommandLineTools/usr/bin/make -C ./hack/api-docs build
docker build -t github.com/external-secrets-mkdocs:latest --build-arg USER_ID=503 -f Dockerfile .
[+] Building 1.3s (9/9) FINISHED                                                                                                 
 => [internal] load build definition from Dockerfile                                                                        0.0s
 => => transferring dockerfile: 74B                                                                                         0.0s
 => [internal] load .dockerignore                                                                                           0.0s
 => => transferring context: 2B                                                                                             0.0s
 => [internal] load metadata for docker.io/library/alpine:3.11                                                              1.2s
 => [auth] library/alpine:pull token for registry-1.docker.io                                                               0.0s
 => [1/3] FROM docker.io/library/alpine:3.11@sha256:bcae378eacedab83da66079d9366c8f5df542d7ed9ab23bf487e3e1a8481375d        0.0s
 => [internal] load build context                                                                                           0.0s
 => => transferring context: 75B                                                                                            0.0s
 => CACHED [2/3] COPY requirements.txt /                                                                                    0.0s
 => CACHED [3/3] RUN apk add -U --no-cache     python3     python3-dev     musl-dev     git     openssh     git-fast-impor  0.0s
 => exporting to image                                                                                                      0.0s
 => => exporting layers                                                                                                     0.0s
 => => writing image sha256:891d6045ae54243a2593039910359851d10ba49a771d8e5f0da0c3e5c46f66ec                                0.0s
 => => naming to github.com/external-secrets-mkdocs:latest                                                                  0.0s
./generate.sh <snip>/hack/api-docs/../..//docs/api/spec.md
I0428 10:16:07.025060    1893 main.go:129] parsing go packages in directory github.com/external-secrets/external-secrets/apis/externalsecrets/v1beta1
I0428 10:16:30.998481    1893 main.go:231] using package=github.com/external-secrets/external-secrets/apis/externalsecrets/v1beta1
I0428 10:16:31.032418    1893 main.go:167] written to <snip>/hack/api-docs/../..//docs/api/spec.md
mkdir -p <snip>/hack/api-docs/../..//site
docker run \
            --mount type=bind,source=<snip>/hack/api-docs/../../,target=/repo \
                --sig-proxy=true \
                --rm \
                --user 503:20 \
                github.com/external-secrets-mkdocs:latest \
                /bin/bash -c "cd /repo && mike deploy --ignore --update-aliases -F hack/api-docs/mkdocs.yml main unstable;"
INFO     -  [macros] - Macros arguments: {'module_name': 'main', 'modules': [], 'include_dir': 'docs/snippets', 'include_yaml': [], 'j2_block_start_string': '', 'j2_block_end_string': '', 'j2_variable_start_string': '', 'j2_variable_end_string': '', 'on_undefined': 'keep', 'verbose': False}
INFO     -  [macros] - Extra variables (config file): ['version', 'analytics']
INFO     -  [macros] - Extra filters (module): ['pretty']
INFO     -  [macros] - Includes directory: docs/snippets
INFO     -  Cleaning site directory
INFO     -  Building documentation to directory: /repo/site
INFO     -  The following pages exist in the docs directory, but are not included in the "nav" configuration:
  - spec.md
  - provider/aws-pushsecret.md
  - snippets/provider-aws-access.md
INFO     -  Documentation built in 7.21 seconds
error: error getting config 'user.name'
make[1]: *** [build] Error 1
make: *** [docs] Error 2

I created a sample that shows service bindings consuming an external secret.

https://github.com/scothis/servicebinding-runtime/blob/external-secrets-sample/samples/external-secrets/README.md

I made progress with make reviewable.

The error: error getting config 'user.name' message is from git looking up the user.name config value. I have the user values defined globally. When the directory is copied into Docker it's detached from the global config and no longer set. Setting user.name and user.email in the local git config unblocked doc generation.

golangci-lint 1.49.0 ended up being comically bad consuming >100GB of RAM before being killed by the OS. The latest version (1.52.2) seems to work fine. That said, make lint reports a number of issues in files I didn't touch. make fmt doesn't actually fix any of the reported issues.

@gusfcarvalho I added the ability to suppress the service binding cluster role

@scothis we just bumped the linter to 1.52 and fixed the linting issues. I'm running the pipeline to see if any other issues appear

for the test case, this block here is similar to what you want to do. Basically add a test definition where tc.checkCondition validates that status.binding.name is the same as spec.target.name (if set), or if it is equals to metadata.name if the spec.target.name is not set :)

I also don't think you need a tc.checkExternalSecret in your case.

The helm test failure also exists on main.

added a PR to fix it @scothis. As soon as we get pipeline clear there I'll merge it.

Sorry for that bad experience 😢

BTW, as part of this PR, because we update CRDs, you'll probably need to bump the helm test snapshots.

This can be done with make helm.test.update. Only the CRDs snapshots should reflect changes 😄

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
0.0% Duplication

@gusfcarvalho rebasing on main was sufficient to fix the failing test for me locally, we'll see what CI says. It looks like only CRD tested is SecretStore. There are no tests for RBAC either.

I'm approving, after the rebase we can just merge.

/approve

Awesome stuff, thanks for this ❤️

Thanks for the support to get this landed.