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

[JENKINS-39867] Add environment variables to container from a secret. #162

Merged
merged 13 commits into from
Aug 14, 2017
Merged

[JENKINS-39867] Add environment variables to container from a secret. #162

merged 13 commits into from
Aug 14, 2017

Conversation

tkgregory
Copy link
Contributor

Previously, environment variables could only be configured as key/value pairs whose value was defined in Jenkins configuration.

This pull request adds the ability to configure an environment variable whose value is derived from a Kubernetes secret. The changes are summarised below:

  • Configure UI so that you can add either a simple or secret environment variable, both for pod and container templates.
  • Change pod template configuration class hierarchy to include PodSecretEnvVar, AbstractPodEnvVar, and AbstractEnvVar. PodEnvVar has not been renamed in order to maintain backwards compatibility with previous versions.
  • Same changes as above for container template configuration class hierarchy.
  • KubernetesCloud now also builds the secret env vars, via a new buildEnvVar method.
  • Functional tests and unit tests to cover all scenarios.

@carlossg
Copy link
Contributor

carlossg commented Jun 1, 2017

Thanks, I see a couple problems

  • public methods being removed, breaking binary compatibility
  • empty space/imports changes that make it difficult to review and can change merge conflicts

@tkgregory
Copy link
Contributor Author

Thanks @carlossg.

  • I have removed the whitespace and the rearrangement of the imports
  • Which public methods are you referring to? It may appear as though the getKey method has been removed from ContainerEnvVar and PodEnvVar but they have actually just moved up to a superclass AbstractEnvVar.

@carlossg
Copy link
Contributor

needs rebase :\

@tkgregory
Copy link
Contributor Author

@carlossg Has been rebased now.

@tkgregory
Copy link
Contributor Author

@carlossg Any chance you could merge this now as I won't be able to work on this for much longer. Thanks.

@@ -287,4 +267,19 @@ public static String substitute(String s, Map<String, String> properties, String
}
return s;
}

private static List<AbstractContainerEnvVar> combineEnvVars(ContainerTemplate parent, ContainerTemplate template) {
List<AbstractContainerEnvVar> combinedSimpleEnvVars = new ArrayList<>();
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 that the name here is misleading, right?

It seems that we are talking about all env vars not just simple, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, it is both types. I have renamed the variables accordingly.

Copy link
Contributor

@iocanel iocanel left a comment

Choose a reason for hiding this comment

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

It looks good to me!

@iocanel
Copy link
Contributor

iocanel commented Aug 2, 2017

@carlossg: Do you mind bringing this in? As you initially requested changes, I'd like to hear you opinion before merging it.

@carlossg
Copy link
Contributor

carlossg commented Aug 5, 2017

I have a working branch with these changes and some more tests, will push in a bit

@carlossg carlossg self-assigned this Aug 6, 2017
@carlossg carlossg changed the title Add environment variables to container from a secret. [JENKINS-39867] Add environment variables to container from a secret. Aug 7, 2017
Tom Gregory and others added 4 commits August 10, 2017 11:50
Run all env var checks under one test

Set the correct syntax for envvars in readme
Use envVar and secretEnvVar for both pods and containers
@carlossg
Copy link
Contributor

I have removed the duplication betwen container and pod envVars and added pipeline tests, leaving envVar for all key/value env vars and secretEnvVar for all coming from a secret.

The only problem right now is that the test fails with Undefined symbol 'envVar' and haven't found yet the cause

mvn clean test -Dtest=KubernetesPipelineTest#runWithEnvVariables

@carlossg
Copy link
Contributor

This is now ready to be merged

@carlossg carlossg merged commit 8597e9b into jenkinsci:master Aug 14, 2017
carlossg added a commit that referenced this pull request Aug 14, 2017
Upgrading from 0.12 causes
class org.csanchez.jenkins.plugins.kubernetes.PodEnvVar is missing its descriptor
carlossg added a commit that referenced this pull request Aug 17, 2017
Fix #162 error PodEnvVar is missing its descriptor
@MattLud
Copy link
Contributor

MattLud commented Aug 18, 2017

Can someone confirm status of this merge?

I am seeing two issues when using master with this branch

@carlossg
Copy link
Contributor

@MattLud can you provide an XML config file that exhibits than problems after upgrade.
Also what version of master are you using?

@carlossg
Copy link
Contributor

You should be using master, not this branch, due to #197

@MattLud
Copy link
Contributor

MattLud commented Aug 19, 2017

I was using latest master when testing with 2.70 and 2.32.1(default pom.xml).

I can get the config.xml on Monday.

@MattLud
Copy link
Contributor

MattLud commented Aug 21, 2017

Looks like the existing variables are no longer deleted.

https://gist.github.com/MattLud/cec8a3a7e63c89beb88f7c59fd8aef4e

Version of code being used rev: 5d4bb7 (today's master)

Steps to recreate:

  1. Checkout 9989fff in fresh workspace
  2. Add cloud and then go add a template with a test environment variable.
  3. Save and stop the jenkins server.
  4. Update to latest master & start server
  5. Go back to configuration, notice the drop down for pod system variables no longer works.

@MattLud
Copy link
Contributor

MattLud commented Aug 21, 2017

Also noticed this in my logs

WARNING: Father of PodEnvVar [getValue()=sample, getKey()=sample] and its getDescriptor() points to two different instances. Probably malplaced @extension. See http://hudson.361315.n4.nabble.com/Help-Hint-needed-Post-build-action-doesn-t-stay-activated-td2308833.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants