Skip to content
This repository has been archived by the owner on Jun 26, 2024. It is now read-only.

Introduce id field in service selector to be used as shortcut in custom environment variables #468

Merged
merged 1 commit into from
Jun 19, 2020

Conversation

isutton
Copy link
Contributor

@isutton isutton commented May 14, 2020

Motivation

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

Changes

#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.

For further more details refer the CONTRIBUTING.md

@openshift-ci-robot
Copy link
Collaborator

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Copy link
Contributor

@pedjak pedjak left a comment

Choose a reason for hiding this comment

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

e2e tests/examples demonstrating the usage of the new field should be added along with the code.

pkg/controller/servicebindingrequest/retriever_test.go Outdated Show resolved Hide resolved
pkg/controller/servicebindingrequest/servicecontext.go Outdated Show resolved Hide resolved
@sbose78
Copy link
Member

sbose78 commented May 14, 2020

I thought we were skipping this piece since we already had a way to refer to backing services as per #407 - however, it's your call team @redhat-developer/service-binding . I'm good with whatever users and the team feels about this! :)

#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.

@isutton do you mean, we are supporting both what was introduced in #407 and what is currently being proposed ? I'm good if that's the case, wanted to check.

@isutton
Copy link
Contributor Author

isutton commented May 18, 2020

I thought we were skipping this piece since we already had a way to refer to backing services as per #407 - however, it's your call team @redhat-developer/service-binding . I'm good with whatever users and the team feels about this! :)

#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.

@isutton do you mean, we are supporting both what was introduced in #407 and what is currently being proposed ? I'm good if that's the case, wanted to check.

@sbose78 indeed, #407 populated the custom environment variable template context with:

  • all collected service data in a hierarchy with transformed names, replacing - to _ so the value can be used in templates like {{ .v1alpha1.postgresql_baiju_dev.Database.db_testing.status.dbCredentials.username }};and
  • all collected data in a hierarchy with the original names.

#468 allows the user to specify an alias (the id property) to be used in the template context, being used like {{ .db_testing.status.dbCredentials.username }}.

Is this clearer, @sbose78?

@sbose78
Copy link
Member

sbose78 commented May 18, 2020

Sounds good, Igor.

@ZhuangYuZY
Copy link

ZhuangYuZY commented May 22, 2020

What is target spec for "id" in SBR ? Any change that before. A SBR CR example will be very help. Thank you.

@codecov
Copy link

codecov bot commented Jun 2, 2020

Codecov Report

Merging #468 into master will increase coverage by 0.06%.
The diff coverage is 66.39%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #468      +/-   ##
==========================================
+ Coverage   64.69%   64.75%   +0.06%     
==========================================
  Files          27       27              
  Lines        1807     1816       +9     
==========================================
+ Hits         1169     1176       +7     
- Misses        472      473       +1     
- Partials      166      167       +1     
Impacted Files Coverage Δ
...r/servicebindingrequest/annotations/annotations.go 0.00% <0.00%> (ø)
...roller/servicebindingrequest/annotations/errors.go 0.00% <0.00%> (ø)
pkg/controller/servicebindingrequest/controller.go 0.00% <0.00%> (ø)
...ontroller/servicebindingrequest/envvars/envvars.go 75.55% <0.00%> (ø)
pkg/controller/servicebindingrequest/watch.go 0.00% <0.00%> (ø)
pkg/controller/servicebindingrequest/common.go 29.41% <30.76%> (ø)
...ller/servicebindingrequest/annotations/resource.go 65.65% <41.37%> (ø)
.../controller/servicebindingrequest/sbrcontroller.go 37.82% <45.45%> (ø)
...ler/servicebindingrequest/annotations/configmap.go 85.71% <50.00%> (ø)
...roller/servicebindingrequest/annotations/secret.go 69.23% <50.00%> (ø)
... and 19 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 a7aa2af...400df4d. Read the comment docs.

@isutton
Copy link
Contributor Author

isutton commented Jun 16, 2020

/retest

1 similar comment
@isutton
Copy link
Contributor Author

isutton commented Jun 16, 2020

/retest

@pedjak
Copy link
Contributor

pedjak commented Jun 17, 2020

@isutton were you able to understand why e2e job is failing on this branch? I see it failing regularly.

@pedjak
Copy link
Contributor

pedjak commented Jun 17, 2020

/retest

@baijum
Copy link
Member

baijum commented Jun 18, 2020

/test e2e

Copy link
Contributor

@pedjak pedjak left a comment

Choose a reason for hiding this comment

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

please squash commits and we are good to go

@pedjak
Copy link
Contributor

pedjak commented Jun 18, 2020

/lgtm

…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.
@isutton
Copy link
Contributor Author

isutton commented Jun 19, 2020

please squash commits and we are good to go

@pedjak force pushed the squashed commits, needs another LGTM.

@pedjak
Copy link
Contributor

pedjak commented Jun 19, 2020

/lgtm

@pedjak pedjak removed their assignment Jun 19, 2020
@pedjak
Copy link
Contributor

pedjak commented Jun 19, 2020

/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pedjak

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

@openshift-merge-robot openshift-merge-robot merged commit 1c56c7d into redhat-developer:master Jun 19, 2020
@sbose78
Copy link
Member

sbose78 commented Jun 20, 2020

@isutton Could you include a sample spec in the PR description?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants