Skip to content

Conversation

@daniel-beck
Copy link
Member

@daniel-beck daniel-beck commented Jul 27, 2020

After feedback from @dwnusbaum in #135 (comment), this is now a much simpler implementation.

Testing notes

Install https://plugins.jenkins.io/generic-environment-filters/ 1.1 or newer, and configure any of the global filters it provides in the global configuration.

  • The easiest to test is "Set environment variable" as it just adds a new environment variable.
  • The most comprehensive test case is "Filter environment variables..." as that allows the definition of job name exclusions, and can be configured to only apply to certain build steps. Both features work with this PR, although the latter is a bit messy because freestyle and pipeline steps have similar names, and show up in the same list here. That's just a https://plugins.jenkins.io/environment-filter-utils/ issue though that shouldn't matter here.

Copy link

@Wadeck Wadeck left a comment

Choose a reason for hiding this comment

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

I imagine that the "Only Keep Specified Environment Variables" specific to Freestyle project is not meant to be used in pipeline, but to be replaced with a pipeline step directly as it was discussed before?

The code is fine, working like a charm.

FTR the plugin "global-environment-filters" is not yet available in the update-center, only from https://repo.jenkins-ci.org/.

Screenshots

When you're a bit too greedy with the regex to remove env variables.
Screenshot_2020-07-27_152641_001

@daniel-beck
Copy link
Member Author

FTR the plugin "global-environment-filters" is not yet available in the update-center, only from https://repo.jenkins-ci.org/.

Sorry, I meant https://plugins.jenkins.io/generic-environment-filters/ , fixed the comment.

@daniel-beck daniel-beck requested a review from dwnusbaum July 29, 2020 08:16
Copy link
Member

@dwnusbaum dwnusbaum left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this approach!

There are some test failures, not sure exactly why, but the test output shows some plugin version conflicts.

Would it be possible to add some basic tests, even if it was just for sh steps?

<revision>2.36</revision>
<changelist>-SNAPSHOT</changelist>
<jenkins.version>2.176.1</jenkins.version>
<jenkins.version>2.248</jenkins.version>
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I guess because of the new interface there is no way to get things working on older versions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it uses a new core type that was added in this core release. But the feature supported here is only available starting with this core release anyway, so that's not a problem.

Supporting older baselines in this plugin might require backports etc. though, so I can understand if you want to hold off the merge for a bit longer.

@dwnusbaum
Copy link
Member

@daniel-beck This looks good to me, but I want to add a basic test to make sure we don't accidentally break this in the future, so I filed daniel-beck#1, could you please take a look at that and merge it into your branch and/or rework as desired?

@daniel-beck
Copy link
Member Author

There are several assertions for text in the build log that only appears in stderr:

  • org.jenkinsci.plugins.workflow.steps.durable_task.ShellStepTest#interruptingAbortsBuildEvenWithReturnStatus
  • org.jenkinsci.plugins.workflow.support.steps.ExecutorStepTest#queueItemAction
  • org.jenkinsci.plugins.workflow.support.steps.ExecutorStepTest#queueTaskVisibility

@dwnusbaum Any idea whether that is expected, perhaps due to a dependency update? I don't want to waste time investigating if you know where this is coming from.

@daniel-beck
Copy link
Member Author

Weird, these tests pass locally on my Mac, with only the assertion in ExecutorStepTest#265 failing.

Copy link

@Wadeck Wadeck left a comment

Choose a reason for hiding this comment

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

Re-tested with other PRs, it still works as expected

@dwnusbaum
Copy link
Member

I'm not sure why the tests are failing, but I will try to look today.

@dwnusbaum
Copy link
Member

I can't reproduce any of the failures locally. My best guess is that they are related to flakiness with the behavior of JenkinsRule.waitForCompletion in relation to the last few lines of log output when Pipelines are finishing, and that they only show up in the CI environment because it's slower than our local machines or something. These issues have been fixed in recent versions of workflow-cps, workflow-job, and jenkins-test-harness, so I refiled this PR with another commit that uses the plugin BOM to update all plugin dependencies in case that helps, see #139.

Due to flakiness around killing processes on Windows (see JENKINS-59152),
we wait until the build has completed before checking that the process on
the agent has completed as well.
Tests that kill agents after waiting for a file to be created were killing
the agent before the sh step had finished launching. The tests now wait
for log output from the sh step to be present before killing the agent.
@dwnusbaum
Copy link
Member

Ok, based on the investigation in #139, I've pushed all of the fixes into this PR. 🤞

@dwnusbaum dwnusbaum merged commit e7e85eb into jenkinsci:master Aug 14, 2020
@frezbo
Copy link

frezbo commented Aug 21, 2020

Getting this error on the latest Jenkins LTS version 2.235.5:

Failed Loading plugin Pipeline: Nodes and Processes v2.36 (workflow-durable-task-step)
2020-08-21T13:31:46.382112094Z java.io.IOException: Failed to load: Pipeline: Nodes and Processes (2.36)
2020-08-21T13:31:46.382115646Z  - Jenkins (2.248) or higher required

This would probably break all LTS versions.

@dwnusbaum
Copy link
Member

dwnusbaum commented Aug 21, 2020

@frezbo You should have seen a warning in the plugin manager before you performed the update warning you that the new version of the plugin requires a newer version of Jenkins. That error is expected if you try to install the plugin on an older version.

@frezbo
Copy link

frezbo commented Aug 21, 2020

@dwnusbaum This was a complete new setup of jenkins using helm with the latest jenkins LTS version. The list of plugins are specified at startup, so there was no way to see the warning.

@dwnusbaum
Copy link
Member

@frezbo Whatever method you are using to install plugins is responsible for performing sanity checks to prevent these kinds of problems. For example, if you are using official Jenkins project docker images, this should be done automatically by install-plugins.sh as long as you are using an LTS version of Jenkins. You should file a bug against whatever packaging mechanism you are using if it is not detecting this kind of issue.

@frezbo
Copy link

frezbo commented Aug 21, 2020

@dwnusbaum I'm using the install-plugins.sh script from the official jenkins docker lts image. And this is the list of plugins passed to the script:

ansicolor:latest
artifactory:latest
authorize-project:latest
configuration-as-code:latest
envinject:latest
git:latest
htmlpublisher:latest
job-dsl:latest
kubernetes-credentials-provider:latest
kubernetes:latest
pipeline-cps-http:latest
rebuild:latest
simple-theme:latest
ssh-agent:latest
uno-choice:latest
workflow-aggregator:latest
workflow-cps-global-lib-http:latest
workflow-cps-global-lib:latest

it seems the script installed an incompatible version, let me know if I'm doing something wrong or missing something.

@daniel-beck
Copy link
Member Author

daniel-beck commented Aug 22, 2020

This is not where this conversation should be happening, it has nothing at all to do with this change. Might be a bug in the Docker image, update site infra, or bad configuration, but not a problem with this plugin.

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