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

ConfigMap Build Sources #19655

Merged

Conversation

adambkaplan
Copy link
Contributor

@adambkaplan adambkaplan commented May 8, 2018

Allow ConfigMaps to be a build source.

Trello Card

RFE 1540978

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 8, 2018
@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 8, 2018
@adambkaplan adambkaplan force-pushed the feature/configmap-build-src branch 3 times, most recently from 4b7d9b6 to 867cc26 Compare May 9, 2018 17:55
@adambkaplan
Copy link
Contributor Author

@adambkaplan
Copy link
Contributor Author

/retest

1 similar comment
@adambkaplan
Copy link
Contributor Author

/retest

@adambkaplan
Copy link
Contributor Author

/assign @bparees

@bparees bparees added this to the v3.11 milestone May 11, 2018
}
return nil
}
func (d *DockerBuilder) copyLocalObject(s localObjectBuildSource, buildDir, mountPath string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

s/buildDir/targetDir/
s/mountPath/sourceDir/

Copy link
Contributor

Choose a reason for hiding this comment

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

(and i'd probably reorder those args to be (src,dst) instead of (dst,src))

@@ -273,6 +321,14 @@ func setupAdditionalSecrets(pod *v1.Pod, container *v1.Container, secrets []buil
}
}

// setupAdditionalConfigMaps creates volume mounts in the given pod for the given list of configMaps
func setupAdditionalConfigMaps(pod *v1.Pod, container *v1.Container, configMaps []buildapi.ConfigMapSpec) {
Copy link
Contributor

Choose a reason for hiding this comment

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

what uses this?

Copy link
Contributor Author

@adambkaplan adambkaplan May 11, 2018

Choose a reason for hiding this comment

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

this is a mirror of the secrets in the Custom Build Strategy. TBH I don't fully understand the use case of this feature, since secrets are supported in the Source part of the BuildConfig

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see anything calling it, so let's remove both of them. (the configmap one and the secret one)

Copy link
Contributor

Choose a reason for hiding this comment

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

oh sorry, i see where it's called. looking.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the custom build strategy secrets field predated our generic "input secrets" support.

I think we should not introduce a configmap equivalent for custom builds, just let people use the "input configmaps" feature to inject configmap content into their custom builds.

mkdir -p "${HOME}/testconfig"
if [[ -f /tmp/configmap/foo ]]; then
# Copy three secrets defined in configmap1 fixture to directory
Copy link
Contributor

Choose a reason for hiding this comment

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

s/secrets/configmap keys/ ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/s/keys/entries/ may be better (map key is the file name, map value is the file content)

Copy link
Contributor

Choose a reason for hiding this comment

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

sure. mostly just trying to not call them secrets since they aren't.

@@ -8,8 +8,15 @@ for s in /tmp/secret? secret?; do
fi
done

for c in /tmp/configmap/* configmap2/*; do
if [[ ! -s "${c}" ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

this makes me realize the check above for secrets is insufficient. it needs to check that the file exists and has a size zero. Currently the check above will pass if the file does not exist at all.

@@ -8,8 +8,15 @@ for s in /tmp/secret? secret?; do
fi
done

for c in /tmp/configmap/* configmap2/*; do
if [[ ! -s "${c}" ]]; then
echo "Found configmap file which should not have been truncated: ${s}"
Copy link
Contributor

Choose a reason for hiding this comment

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

"configmap file is missing or has size zero: "
(and even better, indicate which it is)

@@ -125,5 +125,6 @@ func (bs *CustomBuildStrategy) CreateBuildPod(build *buildapi.Build) (*v1.Pod, e
setupSourceSecrets(pod, &pod.Spec.Containers[0], build.Spec.Source.SourceSecret)
setupInputSecrets(pod, &pod.Spec.Containers[0], build.Spec.Source.Secrets)
setupAdditionalSecrets(pod, &pod.Spec.Containers[0], build.Spec.Strategy.CustomStrategy.Secrets)
setupAdditionalConfigMaps(pod, &pod.Spec.Containers[0], build.Spec.Strategy.CustomStrategy.ConfigMaps)
Copy link
Contributor

Choose a reason for hiding this comment

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

so doesn't this need to be doing a "setupInputConfigMaps" also?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes (had it in my prior branch, missed carrying it over)

@adambkaplan
Copy link
Contributor Author

adambkaplan commented May 11, 2018

@bparees QE brought up a test case injecting configMaps into a Jenkins pipeline build. It's a stretch, but you could define a Secret or ConfigMap with a jenkinsfile, then reference that as the jenkinsfile in the pipeline build.

Worth adding this?

@bparees
Copy link
Contributor

bparees commented May 11, 2018

@bparees QE brought up a test case injecting configMaps into a Jenkins pipeline build. It's a stretch, but you could define a Secret or ConfigMap with a jenkinsfile, then reference that as the jenkinsfile in the pipeline build.

no, not worth taking that on here. that would likely entail changes to the sync plugin anyway.

@adambkaplan
Copy link
Contributor Author

@bparees ptal (api changes held while we continue iterating here)

@bparees
Copy link
Contributor

bparees commented May 14, 2018

lgtm

// build.
ConfigMap kapi.LocalObjectReference

// DestinationDir is the directory where the files from the secret should be
Copy link
Contributor

Choose a reason for hiding this comment

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

"from the secret" -> "from the configMap"

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 29, 2018
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 7, 2018
@adambkaplan adambkaplan changed the title [WIP] ConfigMap Build Sources ConfigMap Build Sources Jun 19, 2018
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 19, 2018
@adambkaplan
Copy link
Contributor Author

@bparees @deads2k rebased & modified commit history - ptal

@bparees
Copy link
Contributor

bparees commented Jun 19, 2018

@adambkaplan did you filter the glide updates or was it really this clean?


// ConfigMaps represents a list of configMaps and their destinations that will
// be used only for the build.
ConfigMaps []ConfigMapBuildSource
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if @deads2k wanted the internal api changes with the update commit, or separate. I think he wanted it separate (ie only generated code in one of the commits, only manual changes in the other commit).

@adambkaplan
Copy link
Contributor Author

/retest

* api: 0ce1df2db7debb15eddb25f3ae76df4180777221
* k8s.io/kubernetes: 6d1dd08538863a8d01b84012b8fdff5b2e6f2240
* update deepcopy and converters
* update swagger and protobuf spec

RFE/bug 1540978
@adambkaplan
Copy link
Contributor Author

@deads2k updated commit history to isolate the new type.

@adambkaplan
Copy link
Contributor Author

I've managed to confuse GitHub's history on this one. Tree in SourceTree is accurate:
screen shot 2018-06-21 at 11 51 50 am

@bparees
Copy link
Contributor

bparees commented Jun 21, 2018

/lgtm

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 21, 2018
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@deads2k
Copy link
Contributor

deads2k commented Jun 21, 2018

API update seems fine.

Please remove the commit for oc, open a different pull, and wait for the rebase. oc is undergoing a massive refactor as part of the rebase to react to upstream changes.

@bparees
Copy link
Contributor

bparees commented Jun 21, 2018

/hold
(for @adambkaplan to remove the new-build commit per @deads2k's request)

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 21, 2018
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 21, 2018
@adambkaplan
Copy link
Contributor Author

/hold cancel
oc changes included in #20064 per @deads2k

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 21, 2018
@bparees
Copy link
Contributor

bparees commented Jun 21, 2018

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 21, 2018
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adambkaplan, bparees

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-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci-robot
Copy link

openshift-ci-robot commented Jun 22, 2018

@adambkaplan: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/openshift-jenkins/unit 86324f6 link /test unit

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@bparees
Copy link
Contributor

bparees commented Jun 22, 2018

/retest

@openshift-merge-robot openshift-merge-robot merged commit 0b10fb1 into openshift:master Jun 22, 2018
@adambkaplan adambkaplan deleted the feature/configmap-build-src branch June 25, 2018 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants