Skip to content

[JENKINS-71509] Clean up of GradleEnterpriseExceptionTaskListenerDecorator #318

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.

Sign up for GitHub

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

Merged
c00ler merged 2 commits into from
Jul 21, 2023
Merged

[JENKINS-71509] Clean up of GradleEnterpriseExceptionTaskListenerDecorator #318

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6d6082f
[JENKINS-71509] Clean up of `GradleEnterpriseExceptionTaskListenerDec…
jglick Jul 11, 2023
0c01119
Reverting changes to `GradleEnterpriseExceptionLogProcessor`; the sec…
jglick Jul 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/main/java/hudson/plugins/gradle/injection/GradleEnterpriseExceptionLogFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ public class GradleEnterpriseExceptionLogFilter extends ConsoleLogFilter impleme
@Override
public OutputStream decorateLogger(Run build, OutputStream logger) {
InjectionConfig injectionConfig = InjectionConfig.get();
if (injectionConfig.isEnabled() && injectionConfig.isCheckForBuildAgentErrors()) {
if (injectionConfig.isEnabled() && injectionConfig.isCheckForBuildAgentErrors() && build != null) {
Copy link
Member Author

@jglick jglick Jul 11, 2023

Choose a reason for hiding this comment

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

Probably unnecessary, but core method does not specify nullability of the build argument, so being defensive.

return new GradleEnterpriseExceptionLogProcessor(logger, build);
}
return logger;
Expand Down
14 changes: 3 additions & 11 deletions src/main/java/hudson/plugins/gradle/injection/GradleEnterpriseExceptionLogProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,12 @@
package hudson.plugins.gradle.injection;

import hudson.console.ConsoleNote;
import hudson.model.Actionable;
import hudson.model.Run;
import hudson.plugins.gradle.AbstractGradleLogProcessor;
import hudson.plugins.gradle.BuildAgentError;
import hudson.plugins.gradle.BuildToolType;

import javax.annotation.Nullable;
import java.io.OutputStream;
import java.nio.charset.Charset;
import java.nio.charset.StandardCharsets;

public final class GradleEnterpriseExceptionLogProcessor extends AbstractGradleLogProcessor {

Expand All @@ -28,13 +24,9 @@ public final class GradleEnterpriseExceptionLogProcessor extends AbstractGradleL

private final BuildAgentErrorListener listener;

public GradleEnterpriseExceptionLogProcessor(OutputStream out, @Nullable Run<?, ?> build) {
this(out, build != null ? build.getCharset() : StandardCharsets.UTF_8, build);
}

public GradleEnterpriseExceptionLogProcessor(OutputStream out, Charset charset, Actionable actionable) {
super(out, charset);
this.listener = new DefaultBuildAgentErrorListener(actionable);
public GradleEnterpriseExceptionLogProcessor(OutputStream out, Run<?, ?> build) {
super(out, build.getCharset());
this.listener = new DefaultBuildAgentErrorListener(build);
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion ...udson/plugins/gradle/injection/GradleEnterpriseExceptionTaskListenerDecoratorFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public GradleEnterpriseExceptionTaskListenerDecorator(Run run) {
@Nonnull
@Override
public OutputStream decorate(@Nonnull OutputStream logger) {
if (isBuildAgentErrorsEnabled()) {
if (run != null) {
Copy link
Member Author

@jglick jglick Jul 11, 2023

Choose a reason for hiding this comment

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

The fix. We cannot call isBuildAgentErrorsEnabled from the agent side, and even if this value were computed on the controller side and serialized, it would not help because run would still have been left null by deserialization and could not be used here. So simply disable the decorator when remoted.

Copy link
Contributor

@alextu alextu Jul 13, 2023

Choose a reason for hiding this comment

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

Could we use a more expressive condition to "know" we're on the controller ? Maybe jenkins.util.JenkinsJVM#isJenkinsJVM ?
As an additional safety net, I think we also need to combine the condition with isBuildAgentErrorsEnabled() (so only executed on the controller) even if it's already called upfront in hudson.plugins.gradle.injection.GradleEnterpriseExceptionTaskListenerDecoratorFactory#of.
Btw, org.jenkinsci.plugins.workflow.log.TaskListenerDecorator.Factory#of implementations are executed only on the controller, right?

Copy link
Member Author

@jglick jglick Jul 13, 2023

Choose a reason for hiding this comment

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

Could we use a more expressive condition to "know" we're on the controller ? Maybe jenkins.util.JenkinsJVM#isJenkinsJVM ?

We could, though it should not matter—if running on an agent, certainly run == null. A checkJenkinsJVM call could be added (inside the run != null block) as a matter of documentation, though it would perhaps just confuse the situation if later you do implement remote decoration (using some data structure that is safe in the agent JVM unlike Run).

we also need to combine the condition with isBuildAgentErrorsEnabled()

Seems redundant since of already runs this check.

#of implementations are executed only on the controller, right?

Right.

return new GradleEnterpriseExceptionLogProcessor(logger, run);
}
return logger;
Expand Down
5 changes: 5 additions & 0 deletions ...le/injection/BuildScanInjectionGradleWithDurableTaskStepUseWatchingIntegrationTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,11 @@ class BuildScanInjectionGradleWithDurableTaskStepUseWatchingIntegrationTest exte
then:
j.assertLogContains('password=****', secondRun)
j.assertLogNotContains(secret, secondRun)

cleanup:
System.err.println('---%<--- agent logs')
agent.computer.logText.writeLogTo(0, System.err)
System.err.println('--->%---')
Comment on lines +79 to +82
Copy link
Member Author

@jglick jglick Jul 11, 2023

Choose a reason for hiding this comment

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

Otherwise you would not notice the errors. Currently JenkinsRule.create[Online]Slave does not stream logs. (InboundAgentRule does.)

Copy link
Contributor

@alextu alextu Jul 13, 2023

Choose a reason for hiding this comment

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

ooh I see, I wondered as well why I could not see the error

}

}
6 changes: 3 additions & 3 deletions src/test/groovy/hudson/plugins/gradle/injection/GradleSnippets.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,19 +32,19 @@ class GradleSnippets {
static WorkflowJob pipelineJobWithCredentials(JenkinsRule j) {
def pipelineJob = j.createProject(WorkflowJob)

pipelineJob.setDefinition(new CpsFlowDefinition("""
pipelineJob.setDefinition(new CpsFlowDefinition('''
stage('Build') {
node('foo') {
withCredentials([string(credentialsId: 'my-creds', variable: 'PASSWORD')]) {
if (isUnix()) {
sh "echo password=\$PASSWORD"
sh 'echo password=$PASSWORD'
Copy link
Member Author

@jglick jglick Jul 11, 2023

Choose a reason for hiding this comment

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

The original test was improperly interpolating the password in Groovy code on the controller, rather than using the environment variable, and so actually running the shell with e.g.

echo password=actual secret here

Pipeline prints a warning about this to the build log. Does not make much difference for purposes of the masking issue (since either way the shell step output contains the secret), but better to test the script that users are actually supposed to write.

Copy link
Contributor

@alextu alextu Jul 13, 2023

Choose a reason for hiding this comment

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

I did not see this warning in the test logs 🤔 but I understand the issue, quite sneaky!

} else {
bat "echo password=%PASSWORD%"
}
}
}
}
""", false))
''', false))
return pipelineJob
}

Expand Down