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

Add handling of working directory #2115

Merged
merged 1 commit into from
Feb 27, 2020
Merged

Conversation

othomann
Copy link
Contributor

Changes

Tekton currently overwrites the working directory in task containers if a specific working directory is not specified. This behaviour is problematic when an image explicitly sets a working directory and expects it to be set to that value upon running.

This PR introduces a feature-flags ConfigMap with a single flag - disable-working-directory-overwrite - that, when set to "true" will prevent Tekton from overriding the working directory in Task containers. This behaviour will be kept for one release which must include a deprecation warning that the behaviour will be changing soon. In the release after that the flag will be flipped so that the default behaviour becomes for Tekton to NOT override the working directory.

Contributes to issue #1836

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

Tekton currently overwrites the working directory in Step containers if not explicitly specified by the user. This behaviour is problematic when an image explicitly sets the working directory and expects it to be set to that value upon running. You can now disable this behaviour in Tekton by using the feature-flags ConfigMap. See docs/install.md for further details on disabling this behaviour.

@googlebot googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Feb 26, 2020
@tekton-robot tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 26, 2020
@skaegi
Copy link
Contributor

skaegi commented Feb 26, 2020

/lgtm
I just tested with my kube-hunter image and now it works without explicitly setting workingDir

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 26, 2020
@skaegi skaegi removed their assignment Feb 26, 2020
pkg/pod/pod.go Outdated Show resolved Hide resolved
@tekton-robot tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 27, 2020
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

I wonder if we can optimize this in the future, as same for the home environment variable, it will add another api call each time we create a pod (adding a bit of latency), for something that might change not that often. We do have a configmap watcher for the default & co, I feel we should have the same for feature flags.

Other than that, looks good to me.

/cc @sbwsg

@tekton-robot tekton-robot requested a review from a user February 27, 2020 06:55
@tekton-robot
Copy link
Collaborator

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 27, 2020
@ghost
Copy link

ghost commented Feb 27, 2020

@vdemeester good point! I took a very quick look at the configmap watcher and got 😨 because I couldn't easily trace where the actual configmaps to watch were being configured. I can take another look at this. Issue to track it: #2117

@ghost
Copy link

ghost commented Feb 27, 2020

/lgtm

@tekton-robot tekton-robot assigned ghost Feb 27, 2020
@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 27, 2020
@tekton-robot tekton-robot merged commit 4c26ba5 into tektoncd:master Feb 27, 2020
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. cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants