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 support for defining Task-wide environment variables #254

Closed
abayer opened this issue Nov 21, 2018 · 14 comments · Fixed by #767
Closed

Add support for defining Task-wide environment variables #254

abayer opened this issue Nov 21, 2018 · 14 comments · Fixed by #767
Assignees
Labels
design This task is about creating and discussing a design help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. meaty-juicy-coding-work This task is mostly about implementation!!! And docs and tests of course but that's a given

Comments

@abayer
Copy link
Contributor

abayer commented Nov 21, 2018

Expected Behavior

In some triggering systems (e.g. Jenkins X) a lot of contextual information is provided via environment variables, for example:

    env:
    - name: DOCKER_REGISTRY
      valueFrom:
        configMapKeyRef:
          key: docker.registry
          name: jenkins-x-docker-registry
    - name: TILLER_NAMESPACE
      value: kube-system
    - name: DOCKER_CONFIG
      value: /home/jenkins/.docker/
    - name: GIT_AUTHOR_EMAIL
      value: [email protected]
    - name: GIT_AUTHOR_NAME
      value: jenkins-x-bot
    - name: GIT_COMMITTER_EMAIL
      value: [email protected]
    - name: GIT_COMMITTER_NAME
      value: jenkins-x-bot
    - name: XDG_CONFIG_HOME
      value: /home/jenkins
    - name: _JAVA_OPTIONS
      value: -XX:+UnlockExperimentalVMOptions -Dsun.zip.disableMemoryMapping=true
        -XX:+UseParallelGC -XX:MinHeapFreeRatio=5 -XX:MaxHeapFreeRatio=10 -XX:GCTimeRatio=4
        -XX:AdaptiveSizePolicyWeight=90 -Xms10m -Xmx192m
    - name: PIPELINE_KIND
      value: release
    - name: REPO_OWNER
      value: abayer
    - name: REPO_NAME
      value: jx-demo-qs
    - name: JOB_NAME
      value: abayer/jx-demo-qs/master
    - name: BRANCH_NAME
      value: master
    - name: JX_BATCH_MODE
      value: "true"

It should be possible to provide all of this information to a PipelineRun and all Tasks ( / TaskRuns) and all of their steps that are invoked by a triggering system that wants to provide this information.

Actual Behavior

Environment variables can only be defined per step, which means that every single step of every single Task that is invoked needs to be explicitly provided with these values.

Additional Info

This is a fine line we tread! If we make global environment variables, then users can use these variables inside of their containers and we will have no idea what they are using and what they aren't.

On the other hand, if we don't, things can become painfully verbose.

🤔

@vdemeester
Copy link
Member

We may want to support that for the pipeline too.

/kind design
/kind enhancement

@bobcatfish
Copy link
Collaborator

/kind design
/kind enhancement

hm i wonder what we need to set up to get that to work - ill add the label manually for now 🤔

@bobcatfish bobcatfish added the design This task is about creating and discussing a design label Nov 27, 2018
@bobcatfish
Copy link
Collaborator

We may want to support that for the pipeline too.

@vdemeester looks like @bkuschel 's request in #263 will take care of that!

@bobcatfish bobcatfish added help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. meaty-juicy-coding-work This task is mostly about implementation!!! And docs and tests of course but that's a given labels Nov 27, 2018
@bobcatfish
Copy link
Collaborator

@abayer would it be possible to add a use case to this?

@abayer
Copy link
Contributor Author

abayer commented Nov 27, 2018

I’ll come up with something concrete tomorrow.

@abayer
Copy link
Contributor Author

abayer commented Nov 27, 2018

Ok, here's an attempt at a use case:

A Pipeline is managed and run by a higher level tool (let's say Jenkins X in this example), in response to GitHub pull requests being opened. Various steps and Tasks within that Pipeline need to have some environment variables made available that will provide GitHub-specific information that can be used in the steps and Tasks to control or modify behavior. A concrete example of this could be passing the PR target repo and branch name, the PR repo's owner, etc. Rather than having to specify these environment variables on each step, it'd be much simpler to be able to specify the environment variables in one place - either on the Task or, if the env vars are relevant across the whole Pipeline, on the Pipeline itself.

This admittedly also depends on the ability to pass in simple key/value string parameters to a PipelineRun, but I feel like that's being addressed elsewhere. If we want to construct an example that isn't concerned with parameterization in general, we could instead go with a Task that contains multiple steps, where each of those steps are running commands that utilize a single environment variable, say, a SonarQube URL. Rather than having to copy the same environment variable onto each step, we could define it (in a hardcoded fashion) on the Task, with the steps picking up that environment variable automatically.

I'd envision this allowing you to override a Pipeline-defined environment variable on a Task or step, or a Task-defined environment variable on a step.

abayer added a commit to abayer/tektoncd-pipeline that referenced this issue Feb 12, 2019
abayer added a commit to abayer/tektoncd-pipeline that referenced this issue Feb 12, 2019
@abayer
Copy link
Contributor Author

abayer commented Feb 12, 2019

/assign abayer

abayer added a commit to abayer/tektoncd-pipeline that referenced this issue Feb 12, 2019
abayer added a commit to abayer/tektoncd-pipeline that referenced this issue Feb 12, 2019
abayer added a commit to abayer/tektoncd-pipeline that referenced this issue Feb 12, 2019
@bobcatfish
Copy link
Collaborator

A concrete example of this could be passing the PR target repo and branch name, the PR repo's owner, etc. Rather than having to specify these environment variables on each step, it'd be much simpler to be able to specify the environment variables in one place - either on the Task or, if the env vars are relevant across the whole Pipeline, on the Pipeline itself.

Hm so info like this would come from the PipelineResources most likely 🤔 I could see making it so that all of the metadata about a Run is made available to the Tasks and steps involved in some form.

Rather than having to copy the same environment variable onto each step, we could define it (in a hardcoded fashion) on the Task, with the steps picking up that environment variable automatically.

hmmm I do kinda feel like params are a better fit for this 🤔- especially since the steps still need to make use of that value somehow, whether it's via $MY_ENV_VAR or via ${inputs.params.my_param}

@abayer
Copy link
Contributor Author

abayer commented Feb 13, 2019

@bobcatfish See my PR at #496 - it properly propagates down the Pipeline env vars to the TaskRun, and then combines any env vars on TaskRun and the Task and adds the whole pile to the steps in the Build that's generated for creating the Pod. That said, switching to env vars being a PipelineResource to make it easier to pass Pipeline env vars down to the TaskRun at reconciliation time without having to just copy them into the TaskRunSpec.

@bobcatfish
Copy link
Collaborator

I have left some incredibly verbose feedback in #496 🙏🙏🙏 Let me know if you want to discuss this more in realtime @abayer

@abayer
Copy link
Contributor Author

abayer commented Feb 14, 2019

Closing this for now after discussion over in #496 and on Slack - for the use case that was driving this for Jenkins X, this is just a nice-to-have to decrease verbosity in the Task CRDs. We can still accomplish everything we need when translating from our own syntax and customizing the CRDs for Jenkins X-specific assumptions by copying the env vars to every step, and there's real value in the CRDs being as unambiguous and explicit as possible.

If anyone else should stumble upon this ticket and has their own reasons for wanting implicit/inherited environment variables supported for Pipelines and/or Tasks, feel free to reopen this!

@abayer abayer closed this as completed Feb 14, 2019
@bobcatfish
Copy link
Collaborator

I've been thinking about this some more - and I've finally come around to the idea of Task level environment variables!

(And I think there's a good chance seeing @abayer 's example translation b/w the Jenkins X yaml and the Task yaml helped convince me XD)

Here is my thought process: If multiple steps in one Task need the same environment variables, making the Task author repeat the environment variables between Tasks doesn't give us any additional clarity, it's just repetitive. So I think we could have top level Task environment variables, which could be overridden by individual steps, and we'd have just as much clarity as we had before, but with less text.

I'd like it to be clear from looking at a Task what environment variables it's using and so:

  • I still don't want to override these at runtime
  • I still don't want to override these at a Pipeline level

Also I think there might be something to think about here whether we want to expose more container spec attributes at the Task level such that they apply to all steps in a Task (similar to how in #714 (comment) I think we might want to make it possible to "template" a pod spec for a Runs)

Also interesting in your thoughts @vdemeester @imjasonh @dlorenc !

@bobcatfish bobcatfish reopened this Apr 16, 2019
@vdemeester
Copy link
Member

Here is my thought process: If multiple steps in one Task need the same environment variables, making the Task author repeat the environment variables between Tasks doesn't give us any additional clarity, it's just repetitive. So I think we could have top level Task environment variables, which could be overridden by individual steps, and we'd have just as much clarity as we had before, but with less text.

Agreed !

I'd like it to be clear from looking at a Task what environment variables it's using and so:

* I still don't want to override these at runtime

* I still don't want to override these at a Pipeline level

Agreed too ! 👼

Also I think there might be something to think about here whether we want to expose more container spec attributes at the Task level such that they apply to all steps in a Task (similar to how in #714 (comment) I think we might want to make it possible to "template" a pod spec for a Runs)

That's a good point, we would do more or less the same as deployment.template (or job.template) then 👼

@bobcatfish
Copy link
Collaborator

deployment.template (or job.template)

ooo neat, TIL!

chmouel pushed a commit to chmouel/tektoncd-pipeline that referenced this issue Dec 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design This task is about creating and discussing a design help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. meaty-juicy-coding-work This task is mostly about implementation!!! And docs and tests of course but that's a given
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants