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

Streamline data retrieval based on annotations #407

Merged
merged 1 commit into from
May 13, 2020

Conversation

isutton
Copy link
Contributor

@isutton isutton commented Mar 31, 2020

fixes https://issues.redhat.com/browse/APPSVC-493

Motivation

  • Have a test baseline covering existing service binding annotation semantics; and
  • Provide configuration values from all declared services to the custom environment variable templates.

Changes

Some existing components have been simplified and several others have been removed in order to reduce the number of indirections in the system.

The annotations package has been created to group all annotation related symbols. annotations.Handler is an interface specifying the annotation handler contract, which is responsible for processing a single annotation key and value pairs as for example .../status.dbConnectionIP and binding:env:attribute; an instance can be obtained through annotations.BuildHandler().

There are currently two annotation handlers: annotations.AttributeHandler and annotations.ResourceHandler. The handlers return a Result containing the data produced by the annotation in Data, which is stored in a deep structure with the root path found in the Path field. Additionally the binding type (whether volume mounts or environment variables should be used) is present in the Type field.

The AttributeHandler is used to collect a value from the object where the annotation is declared; for example, in the example .../status.dbConnectionIP and binding:env:attribute annotation pair the data found in the simple JSONPath status.dbConnectionIP will be extracted from the object. On the other hand, the ResourceHandler is used to collect information present in another external resource, for example a config map or a secret; in the .../status.dbCredentials-user and binding:env:object:configmap annotation pairs example, the data.user field from the config map resource which name is present in status.dbCredentials will be extracted.

The envvars package contains tooling to construct map[string][]byte data-structures from an arbitrarily nested map[string]interface{} value.

The nested package contains utilities to extract the arbitrarily deep data-structures in the annotation handlers.

Since the whole data extraction process has been modified to provide the testing baseline, this change also includes support for custom environment variable templates such as {{ index "v1alpha1" "postgresql.baiju.dev" "Database" "db-testing" "metadata" "name" }} and {{ .v1alpha1.postgresql_baiju_dev.Database.db_testing.metadata.name }}.

Test coverage should be improved, and internals should keep to be simplified in subsequent work units, such as:

  • Review whether ServiceBinder and Binder can be further simplified.
  • Review whether the data being propagated through function parameters in the reconciliation can be re-grouped now that some indirections have been removed.

Testing

Currently the only tests being amended are unit tests. Once integration is more mature, e2e tests will be used to guarantee the behavior hasn't been changed unwillingly.

For further more details refer the CONTRIBUTING.md

@sbose78
Copy link
Member

sbose78 commented Mar 31, 2020

Thank you!

@codecov
Copy link

codecov bot commented Apr 2, 2020

Codecov Report

Merging #407 into master will decrease coverage by 3.18%.
The diff coverage is 69.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #407      +/-   ##
==========================================
- Coverage   66.26%   63.07%   -3.19%     
==========================================
  Files          21       27       +6     
  Lines        1479     1774     +295     
==========================================
+ Hits          980     1119     +139     
- Misses        377      489     +112     
- Partials      122      166      +44     
Impacted Files Coverage Δ
...r/servicebindingrequest/annotations/annotations.go 0.00% <0.00%> (ø)
...roller/servicebindingrequest/annotations/errors.go 0.00% <0.00%> (ø)
pkg/controller/servicebindingrequest/binder.go 63.75% <ø> (-0.88%) ⬇️
pkg/controller/servicebindingrequest/controller.go 0.00% <0.00%> (ø)
.../controller/servicebindingrequest/nested/nested.go 59.03% <59.03%> (ø)
pkg/controller/servicebindingrequest/reconciler.go 61.44% <59.18%> (+14.87%) ⬆️
...controller/servicebindingrequest/servicecontext.go 59.80% <59.80%> (ø)
...ller/servicebindingrequest/annotations/resource.go 65.65% <65.65%> (ø)
...roller/servicebindingrequest/annotations/secret.go 69.23% <69.23%> (ø)
.../controller/servicebindingrequest/servicebinder.go 64.37% <75.00%> (ø)
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 91d0145...c141172. Read the comment docs.

@sbose78
Copy link
Member

sbose78 commented Apr 2, 2020

Could you add a description and title on what to expect in the code, please?

@sbose78
Copy link
Member

sbose78 commented Apr 6, 2020

Could you limit this PR to only changes to custom env var handling ? There are a lot of discussions on annotations going on ( will link), and it would be nice to have them settle before we introduce new things :)

@isutton
Copy link
Contributor Author

isutton commented Apr 7, 2020 via email

@sbose78
Copy link
Member

sbose78 commented Apr 9, 2020

I want to review this bad but I'd like to have a description :-)

@pradeepto
Copy link

@isutton Can we please have proper description for this PR?

@isutton isutton changed the title APPSVC-493 Streamline data retrieval based on annotations Apr 14, 2020
pkg/controller/servicebindingrequest/nested/acc/acc.go Outdated Show resolved Hide resolved
pkg/controller/servicebindingrequest/nested/nested.go Outdated Show resolved Hide resolved
pkg/controller/servicebindingrequest/planner.go Outdated Show resolved Hide resolved
pkg/controller/servicebindingrequest/planner.go Outdated Show resolved Hide resolved
pkg/controller/servicebindingrequest/planner.go Outdated Show resolved Hide resolved
pkg/controller/servicebindingrequest/planner.go Outdated Show resolved Hide resolved
@isutton isutton force-pushed the APPSVC-493 branch 3 times, most recently from 2f27151 to e6fce56 Compare April 18, 2020 23:42
@sbose78
Copy link
Member

sbose78 commented Apr 22, 2020

@redhat-developer/service-binding Please review.

@isutton
Copy link
Contributor Author

isutton commented Apr 24, 2020

/retest

@isutton isutton marked this pull request as ready for review April 24, 2020 20:31
Makefile Outdated Show resolved Hide resolved
@baijum
Copy link
Member

baijum commented May 13, 2020

/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: baijum

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@baijum
Copy link
Member

baijum commented May 13, 2020

/lgtm

@openshift-merge-robot openshift-merge-robot merged commit 179c2fb into redhat-developer:master May 13, 2020
@otaviof
Copy link
Member

otaviof commented May 13, 2020

Could you please add comments on why those review comments cannot be addressed in this PR ?

Sorry for the late reply on this, comments should have been addressed before merging! But, since in the cabal meeting of yesterday, we've decided to have this merged and address the open comments in dedicated issues/PRs:

Why can't we avoid exposing functions/variables which we don't have to, in this PR ?

We've spoken about this yesterday. But, let's make sure #462 gets priority, so we can address this concern promptly.

@otaviof
Copy link
Member

otaviof commented May 13, 2020

If I may summarize what we've discussed yesterday, the points were:

  • the PR is good as it's to be merged, having a squash of its commits and all the CI jobs passing;
  • we would go through all open comments and make sure they are reflected on issues, to be followed up right after;
  • we would make sure the current work is also addressed in the sprint starting today;

Please correct if I'm missing something, and lets all make sure concerts are addressed properly.

@isutton
Copy link
Contributor Author

isutton commented May 13, 2020

Since most of the issues pointed out are related to either a) error handling, b) package visibility or c) missing coverage, I've created issues with links to those comments to deal with those.

Typically, we add a comment with the link to the postponed ticket that would address it with a comment with why it need not be addressed in the current PR. Otherwise, there's no way for the PR reviewer or a passer-by to understand why a specific comment doesn't have a follow up - yet marked as 'resolved'.

Could you please add comments on why those review comments cannot be addressed in this PR ?

They could have, but I considered those less urgent than enabling developers needing to work in the project; instead I created GitHub issues grouping related review comments

This has been first mentioned after I started working on this branch, so it is expected that using internals as libraries hasn't been the focus of this work.

Irrespective of whether this would be used as a library, we should avoid exposing functions/variables if they don't need to be exposed. It's a best practice that should be followed irrespective of what the focus of a PR is. I wouldn't be comfortable if we introduced tech debt - without a solid reason to do so.

Why can't we avoid exposing functions/variables which we don't have to, in this PR ?

Same as above, they could have, but I considered those less urgent.

Now that the PR is merged, I will do the cleanup meanwhile the rest of the team can do more interesting things such as implement the Service Binding Spec annotations, or make sure errors and warnings are being properly surfaced to the user through proper channels such as events and conditions.

Hope I have informed you well :)

isutton pushed a commit to isutton/service-binding-operator that referenced this pull request Jun 4, 2020
This change adjusts the visibility of symbols in the entire project; the
visibility has been raised several moments during review of redhat-developer#407 and
tracked in redhat-developer#462 after redhat-developer#407 has been merged.

Because of the nature of the change, some variables have been renamed to
avoid value shadowing when the variable name is identical to the type
name (e.g. 'binder := &binder{}').

Closes redhat-developer#462
isutton pushed a commit to isutton/service-binding-operator that referenced this pull request Jun 5, 2020
This change adjusts the visibility of symbols in the entire project; the
visibility has been raised several moments during review of redhat-developer#407 and
tracked in redhat-developer#462 after redhat-developer#407 has been merged.

Because of the nature of the change, some variables have been renamed to
avoid value shadowing when the variable name is identical to the type
name (e.g. 'binder := &binder{}').

Closes redhat-developer#462
isutton pushed a commit to isutton/service-binding-operator that referenced this pull request Jun 8, 2020
This change adjusts the visibility of symbols in the entire project; the
visibility has been raised several moments during review of redhat-developer#407 and
tracked in redhat-developer#462 after redhat-developer#407 has been merged.

Because of the nature of the change, some variables have been renamed to
avoid value shadowing when the variable name is identical to the type
name (e.g. 'binder := &binder{}').

Closes redhat-developer#462
isutton pushed a commit to isutton/service-binding-operator that referenced this pull request Jun 8, 2020
This change adjusts the visibility of symbols in the entire project; the
visibility has been raised several moments during review of redhat-developer#407 and
tracked in redhat-developer#462 after redhat-developer#407 has been merged.

Because of the nature of the change, some variables have been renamed to
avoid value shadowing when the variable name is identical to the type
name (e.g. 'binder := &binder{}').

Closes redhat-developer#462
isutton pushed a commit to isutton/service-binding-operator that referenced this pull request Jun 8, 2020
This change adjusts the visibility of symbols in the entire project; the
visibility has been raised several moments during review of redhat-developer#407 and
tracked in redhat-developer#462 after redhat-developer#407 has been merged.

Because of the nature of the change, some variables have been renamed to
avoid value shadowing when the variable name is identical to the type
name (e.g. 'binder := &binder{}').

Closes redhat-developer#462
isutton pushed a commit to isutton/service-binding-operator that referenced this pull request Jun 19, 2020
…om environment variables

With the configuration values collection and aggregation solved in redhat-developer#407, this
change introduces the id field in backing service selectors to be used as
alias while composing custom environment variables.

Solves redhat-developer#396

PR redhat-developer#407 has grouped service configuration values in the template context
using the following the version, api group, kind and name (for example
{{ .v1alpha1.postgresql_baiju_dev.Database.db_testing.status.dbConnection }}).

This change groups service configuration values in the template context using
the optional id field declared by the user, allowing the user to write
{{ .db_testing.status.dbConnection }} instead.
openshift-merge-robot pushed a commit that referenced this pull request Jun 19, 2020
…om environment variables (#468)

With the configuration values collection and aggregation solved in #407, this
change introduces the id field in backing service selectors to be used as
alias while composing custom environment variables.

Solves #396

PR #407 has grouped service configuration values in the template context
using the following the version, api group, kind and name (for example
{{ .v1alpha1.postgresql_baiju_dev.Database.db_testing.status.dbConnection }}).

This change groups service configuration values in the template context using
the optional id field declared by the user, allowing the user to write
{{ .db_testing.status.dbConnection }} instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet