Add ConfigMap volume source support to workspaces #1800
Conversation
googlebot
added
the
cla: yes
tekton-robot
added
the
size/L
ghost
requested a review
from 
|
The following is the coverage report on pkg/.
|
|
The following is the coverage report on pkg/.
|
|
The following is the coverage report on pkg/.
|
ghost
mentioned this pull request
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vdemeester 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 |
tekton-robot
added
the
approved
|
Rebased on top of v1alpha2 changes. This is ready for review. |
|
The following is the coverage report on pkg/.
|
One of the original feature requests for workspaces was to include support for ConfigMaps as the contents of a volume mounted into Task containers. This PR introduces support for ConfigMaps as workspaces in a TaskRun definition.
|
The following is the coverage report on pkg/.
|
|
The following is the coverage report on pkg/.
|
| configmap: | ||
| name: my-configmap | ||
| ``` | ||
|
|
| name: my-configmap | ||
| data: | ||
| message: hello world | ||
| --- |
| @@ -24,5 +24,5 @@ import ( | |||
| type WorkspaceDeclaration = v1alpha2.WorkspaceDeclaration | |||
|
|
|||
| // WorkspaceBinding maps a Task's declared workspace to a Volume. | |||
| // Currently we only support PersistentVolumeClaims and EmptyDir. | |||
| // Currently we only support PersistentVolumeClaims, EmptyDir and ConfigMap. | |||
There was a problem hiding this comment.
I wonder if we'd be better off removing this comment so we don't have it fall out of date later 🤔
| "workspace.persistentvolumeclaim", | ||
| "workspace.emptydir", | ||
| "workspace.configmap", | ||
| } |
| // For a ConfigMap to work, you must provide the name of the ConfigMap to use. | ||
| if b.ConfigMap != nil && b.ConfigMap.LocalObjectReference.Name == "" { | ||
| return apis.ErrMissingField("workspace.configmap.name") | ||
| } |
There was a problem hiding this comment.
should we have a unit test that tries to set a configmap + something else?
that's quickly starting to balloon out of control unit test permutation wise - i wonder if we can isolate the logic for determining this somehow such that the tests wont need to be updated with every permutation 🤔
|
/lgtm We can always tackle that invalid case unit test thing later |
Changes
One of the original feature requests for workspaces was to include
support for ConfigMaps as the contents of a volume mounted into
Task containers.
This PR introduces support for ConfigMaps as workspaces in a TaskRun
definition.
This PR contributes to #1438
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
Reviewer Notes
If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.
Release Notes