-
Notifications
You must be signed in to change notification settings - Fork 48
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 a Datadog build step #88
Conversation
a5f7360
to
55453e8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good so far! I'll manually test it soon. There should definitely be docs for this, though (they can be in a separate PR if it's a lot to add).
} | ||
} | ||
invoker.start(); | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the return value used for here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That step is performing work that is not yet done here
ExtensionList.clearLegacyInstances(); | ||
cfg.setCollectBuildLogs(false); | ||
EnvVars env = new EnvVars(); | ||
env.put("DD_TEST", "bar"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this for? Shouldn't it not be needed for the integration test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, can be removed totally. I was hesitating between putting the env var here or in the pipeline definition.
}; | ||
stubClient.assertMetric("jenkins.job.duration", hostname, expectedTags); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a test for multiple tags and setting collectLogs: false
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extending the current one instead if that's ok
import org.kohsuke.stapler.DataBoundSetter; | ||
|
||
/** | ||
* Pipeline plug-in step for recording time-stamps. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem accurate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! One scenario I tested was creating a pipeline with multiple stages and adding new tags to only one stage, making sure only that stage had the new tags, which could be a good unit test.
Is there any documentation on how to use this? I tried copying the examples from the unit tests but I get Happy to open an issue if that's the right course of action here. |
Hey @stpierre |
Adding a pipeline compatible build steps in order to: