Skip to content

Conversation

@daniel-beck
Copy link
Member

See JENKINS-62014.

This adds support for global build step environment filters to durable task based pipeline steps.

Downstream from jenkinsci/jenkins#4683 and jenkinsci/durable-task-plugin#124.

@jeffret-b
Copy link

There's a build failure issue with dependencies.

@jeffret-b
Copy link

This looks good, though I'm not an expert in this area. There's a build error with dependencies so I'm not able to try it out or go further with it.

Copy link

@jeffret-b jeffret-b left a comment

Choose a reason for hiding this comment

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

These two incrementals might do the job. I'm not too confident in the durable-task one.

@jeffret-b
Copy link

I tested basic operations with a few different configurations. Everything worked as expected.

The console log messages are a little weird, but it looks like they don't originate here.

This PR could use an improved description of what it adds and how it needs to be tested.

ws = context.get(FilePath.class);
node = FilePathUtils.getNodeName(ws);
DurableTask durableTask = step.task();
durableTask.setRun(context.get(Run.class));
Copy link
Member

Choose a reason for hiding this comment

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

@daniel-beck It looks like a few lines down in this method, you would have access to the launcher, so you could call launcher.prepareFilterRules(context.get(Run.class), durableTask); (not sure what context makes the most sense, could pass durableTask or this I guess...) without needing to make any changes in durable-task (other than the addition of implements EnvVarsFilterableBuilder on FileMonitoringTask, but I am not totally clear on whether it needs to be there or could be here on DurableTaskStep).

Any particular reason you wanted to make changes to each kind of task over in durable-task? I only glanced at the Jenkins core PR so may very well be missing something obvious.

@daniel-beck
Copy link
Member Author

Superseded by #137.

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.

3 participants