docs: document workload resource mapping

[sadlerap]

/kind documentation

Signed-off-by: Andy Sadler [email protected]

Changes

Adds documentation for ClusterWorkloadResourceMapping.servicebinding.io resources.

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

• Docs
  included if any changes are user facing
• Tests
  included if any functionality added or changed. For bugfixes please include tests that can catch regressions
• All acceptance test scenarios included in the PR which verifies a bugfix or a requested feature reported by a non-member are tagged with @external-feedback tag.
• Follows the commit message standard

[baijum]

/lgtm

[sadlerap]

/retest

[sadlerap]

/retest

[sadlerap]

/retest

[Srivaralakshmi]

@sadlerap: Left some suggestions and questions. PTAL and respond. Thanks!

[Srivaralakshmi]

Suggested change

	Most of these fields are optional, and SBO assumes PodSpec-compatible defaults when not specified.
	SBO requires that each of these fields is semantically equivalent to the corresponding field in a Pod
	deployment; failing to maintain this may result in unexpected behavior.
	Most of these fields are optional. When not specified, the {servicebinding-title} assumes defaults that are compliant with the `PodSpec workload resources.
	+
	[NOTE]
	====
	Each of these fields must be semantically equivalent to the corresponding field in a pod deployment.
	deployment. Otherwise, this might result in unexpected behavior.

Question:

• Can you state an example of what such unexpected behavior might look like for general understanding purposes?
• Unexpected behavior of what? Please clarify.

[sadlerap]

If a workload resource is not semantically compatible with a pod template, we can't make any guarantees about the behavior a user observes. This statement is meant to be a warning along those lines.

[sadlerap]

On consideration, I've decided to reword this warning more toward the need for structural equivalence. I think talking about semantic equivalence is less important structural compatibility. If structural compatibility is not maintained, SBO will throw an error when binding is attempted, and users of SBO should be aware of this limitation.

[pmacik]

/retest

[pmacik]

/lgtm

[codecov]

Codecov Report

Merging #1139 (81099de) into master (65ea52d) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1139   +/-   ##
=======================================
  Coverage   57.84%   57.84%           
=======================================
  Files          31       31           
  Lines        2697     2697           
=======================================
  Hits         1560     1560           
  Misses        976      976           
  Partials      161      161           

---

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 65ea52d...81099de. Read the comment docs.

[sadlerap]

/retest

[sadlerap]

/retest

[Srivaralakshmi]

@sadlerap Please incorporate the review comments suggested. Thanks!

[Srivaralakshmi]

Suggested change

	<7> Optional: Identifier of the volume mounts of a pod lives. The default value is `.volumeMounts`.
	<7> Optional: Custom location of the volumes with a fixed JSONPath. The default value is `.spec.template.spec.volumes`.

My understanding is that these are the custom locations/paths where the volume mounts reside. So should be stated explicitly and consistently. If it is otherwise, please clarify by giving an example of what it will actually look like. And we can work together to word it out for the user.

[sadlerap]

The default value for this field is .volumeMounts, not .spec.template.spec.volumes. This comes from the spec.

I've adjusted the wording here to be more consistent with the other optional fields. PTAL.

[Srivaralakshmi]

Suggested change

	<8> Optional: where volumes live, specified with a fixed JSONPath. The defaults value is `.spec.template.spec.volumes`.
	<8> Optional: Custom location of the volumes with a fixed JSONPath. The default value is `.spec.template.spec.volumes`.

Same comment as above.

[sadlerap]

I'm not a fan of this wording, since someone could read the first sentence as the volumes having the fixed jsonpath, which isn't correct. I'm hoping to avoid unambiguous syntax here.

Thoughts on this?

Suggested change

	<8> Optional: where volumes live, specified with a fixed JSONPath. The defaults value is `.spec.template.spec.volumes`.
	<8> Optional: Custom location of the volumes field, specified with a fixed JSONPath. The defaults value is `.spec.template.spec.volumes`.

[sadlerap]

@Srivaralakshmi thoughts?

[sadlerap]

@Srivaralakshmi I've made changes from your suggestions. A few things:

• The suggestions that were applied verbatim have been marked as resolved.
• The items I left open had issues I wanted another pass on before I resolved (either the wording wasn't accurate, or I made different changes from the ones suggested). Let me know what you think.

[Srivaralakshmi]

Suggested change

	The specification also provides a way of defining exactly where binding data needs to be projected into. This method may be used when the preceding methods would not be able to configure custom path locations correctly. You may define `ClusterWorkloadResourceMapping.servicebindings.io` resources, which specifies where binding data will be projected for a given workload kind.
	The specification also provides a way to define exactly where binding data needs to be projected. Use this method when you are not able to configure custom path locations correctly by previous methods. You can define the `ClusterWorkloadResourceMapping` resources, which specify where binding data is to be projected for a given workload kind.

• User defines the custom location, so make the sentences more direct.
• into is not required. Hence dismissing.
• Avoid may; for specifying probability use might.
• Avoid passive sentences.
• I have suggested what I would write for the user.
• Please refer to the RH writing guidelines for usage of will, can, may, might, and also passive sentences.
• It is ClusterWorkloadResourceMapping and not ClusterWorkloadResourceMapping.servicebindings.io, cause this feature applies to both API groups. So suggest keeping it neutral.

[Srivaralakshmi]

@sadlerap Any specific reason why the suggested changes are not applied here? I do not also see a comment from you as you had mentioned highlighting the changes you needed to make.

[Srivaralakshmi]

Suggested change

	<4> Identifies where containers live, specified with a JSONPath. If no entries are defined, the {servicebinding-title} defaults to two paths: `.spec.template.spec.containers` and `.spec.template.spec.initContainers`, with all other fields set as their default.
	<4> Identifier of the `.containers` field in a pod, specified with a JSONPath. If no entries are defined, the {servicebinding-title} defaults to two paths: `.spec.template.spec.containers` and `.spec.template.spec.initContainers`, with all other fields set as their default.

[Srivaralakshmi]

@sadlerap Any specific reason why the suggested changes are not applied here? I do not also see a comment from you as you had mentioned highlighting the changes you needed to make.

[Srivaralakshmi]

Left some suggestions, PTAL

[Srivaralakshmi]

@sadlerap PTAL and ensure to apply all the changes suggested. If there is a different perspective, please set up a meeting with me so that we can discuss and resolve it at the earliest. Thanks!

[Srivaralakshmi]

Suggested change

	* Most of these fields are optional, and the {servicebinding-title} assumes PodSpec-compatible defaults when they are not specified.
	* Most of these fields are optional, and when not specified, the {servicebinding-title} assumes defaults that are compatible with the `PodSpec workload resources.

Suggest writing sentences that give complete details. Avoid ambiguous pronouns - they.

[sadlerap]

I thought the usage of they in this context was clear. Would this be better?

Suggested change

	* Most of these fields are optional, and the {servicebinding-title} assumes PodSpec-compatible defaults when they are not specified.
	* Most of these fields are optional. When they are not specified, the {servicebinding-title} assumes defaults compatible with `Pod` resources.

[Srivaralakshmi]

Suggested change

	[NOTE]
	====
	There is some behavior in the CoreOS API that does not exist in the specification API:
	. If a `ServiceBinding` resource with `bindAsFiles: false` is created with one of these mappings, then environment variables will be projected into `.envFrom` underneath each `path` field specified in the corresponding `ClusterWorkloadResourceMapping` resource.
	. Both `ClusterWorkloadResourceMapping.servicebinding.io` resources and the `.spec.application.bindingPath.containersPath` field will be considered for binding purposes. This behavior would be equivalent to adding an entry to the corresponding `ClusterWorkloadResourceMapping` resource with `path: $containersPath`, with all other values taking their default value.
	====
	[NOTE]
	====
	The following behavior occurs only in the `binding.operators.coreos.com` API group:
	. If a `ServiceBinding` resource with the `bindAsFiles: false` flag value is created together with one of these mappings, then environment variables are projected into the `.envFrom` field underneath each `path` field specified in the corresponding `ClusterWorkloadResourceMapping` resource.
	. As a cluster administrator, you can specify both a `ClusterWorkloadResourceMapping` resource and the `.spec.application.bindingPath.containersPath` field in a `ServiceBinding.bindings.coreos.com` resource for binding purposes. The {servicebinding-title} attempts to project binding data into the locations specified in both of these methods. This behavior is equivalent to adding a container entry to the corresponding `ClusterWorkloadResourceMapping` resource with the `path: {containersPath}` attribute, with all other values taking their default value.
	====

The Verbatim is marked as fixed and resolved, but I am not seeing the suggested changes. Hence suggesting once again. It was already there in the comment given previously. Ensure to apply the changes suggested as is, as the review comments are given from the language guidelines perspective that must adhere to RH standards.

[sadlerap]

I can't use path: {containersPath}, since asciidoctor correctly complains that the {containersPath} reference is not defined.

Suggested change

	[NOTE]
	====
	There is some behavior in the CoreOS API that does not exist in the specification API:
	. If a `ServiceBinding` resource with `bindAsFiles: false` is created with one of these mappings, then environment variables will be projected into `.envFrom` underneath each `path` field specified in the corresponding `ClusterWorkloadResourceMapping` resource.
	. Both `ClusterWorkloadResourceMapping.servicebinding.io` resources and the `.spec.application.bindingPath.containersPath` field will be considered for binding purposes. This behavior would be equivalent to adding an entry to the corresponding `ClusterWorkloadResourceMapping` resource with `path: $containersPath`, with all other values taking their default value.
	====
	[NOTE]
	====
	There is some behavior in the CoreOS API that does not exist in the specification API:
	. If a `ServiceBinding` resource with `bindAsFiles: false` is created with one of these mappings, then environment variables will be projected into `.envFrom` underneath each `path` field specified in the corresponding `ClusterWorkloadResourceMapping` resource.
	. As a cluster administrator, you can specify both a `ClusterWorkloadResourceMapping` resource and the `.spec.application.bindingPath.containersPath` field in a `ServiceBinding.bindings.coreos.com` resource for binding purposes. The {servicebinding-title} attempts to project binding data into the locations specified in both of these methods. This behavior is equivalent to adding a container entry to the corresponding `ClusterWorkloadResourceMapping` resource with the `path: $containersPath` attribute, with all other values taking their default value.
	====

[sadlerap]

@Srivaralakshmi I've incorporated your changes. For things I couldn't apply verbatim, I left a comment highlighting the changes I needed to make. Let me know what you think.

[Srivaralakshmi]

@sadlerap Thanks for the change. But I am not seeing the suggested changes 1 and 2 applied. Ensure to apply the changes suggested as is, as the review comments are given from the language guidelines perspective that must adhere to RH standards.

[Srivaralakshmi]

Suggested change

	* Most of these fields are optional. When they are not specified, the {servicebinding-title} assumes defaults compatible with `Pod` resources.
	* Most of these fields are optional. When they are not specified, the {servicebinding-title} assumes defaults compatible with `Pod` resources.

[Srivaralakshmi]

@sadlerap Thanks for the change. Just remove the extra space.

[Srivaralakshmi]

@sadlerap Are these Pod resources or PodSpec resources? Please clarify.

[Srivaralakshmi]

@sadlerap Any specific reason why the suggested changes are not applied here? I do not also see a comment from you as you had mentioned highlighting the changes you needed to make.

[Srivaralakshmi]

@sadlerap Any specific reason why the suggested changes are not applied here? I do not also see a comment from you as you had mentioned highlighting the changes you needed to make.

[Srivaralakshmi]

@sadlerap Have a few suggestions and queries. PTAL.

[Srivaralakshmi]

Question: Is this admonition only for L172?

Suggestion: If yes, then please put + in L173, so that the admonition is indented under the Example section.

[sadlerap]

The [IMPORTANT] section is supposed to be for the entire list of callouts preceding it, not just for the last one.

[sadlerap]

@Srivaralakshmi Something I just noticed: my changes are under docs/userguide/modules/creating-service-bindings. However, as of #1142, its nav.adoc was moved to docs/userguide/modules/binding-workloads-using-sbo. This means that when I attempt to render my changes, they don't show up.

I've updated both this PR and #1149 to move my changes to the latter directory. Let me know if there's anything else I need to change.

[Srivaralakshmi]

Suggested change

	* You must namespace `ClusterWorkloadResourceMapping` resources under the `servicebinding.io` API group. However, the `ClusterWorkloadResourceMapping` resources interact with `ServiceBinding` resources under both the `binding.operators.coreos.com` and `servicebinding.io` API groups.
	* You must namespace `ClusterWorkloadResourceMapping` resources only under the `servicebinding.io` API group. However, the `ClusterWorkloadResourceMapping` resources interact with `ServiceBinding` resources under both the `binding.operators.coreos.com` and `servicebinding.io` API groups.

Comments;

1. Removed extra space before "However,"
2. Please add "only" also

@sadlerap These are the only comments. Rest LGTM.

[sadlerap]

I've migrated to single-spacing between sentences for consistency.

I don't see how point 2 is relevant, since must earlier in the sentence implies no other option is available. That said, I've made the change.

[Srivaralakshmi]

Suggestion: Use bulleted list

[sadlerap]

Fixed.

[Srivaralakshmi]

Suggested change

	* Most of these fields are optional. When they are not specified, the {servicebinding-title} assumes defaults compatible with `PodSpec` resources.
	* Most of these fields are optional. When they are not specified, the {servicebinding-title} assumes defaults compatible with `PodSpec` resources.

Removed extra space before "When"

[sadlerap]

Fixed.

[Srivaralakshmi]

@sadlerap Thanks for the changes. Approved!

[Srivaralakshmi]

/lgtm
/approved

[sadlerap]

/retest

[dperaza4dustbit]

/approve

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dperaza4dustbit

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:

• OWNERS [dperaza4dustbit]

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