Repository navigation
Apply the Datadog Jenkins plugin setup to Jenkinsfiles - #271
juan-fernandez wants to merge 5 commits into
Conversation
onboard --apply now enables the Datadog Jenkins plugin in the Jenkinsfiles
whose host-agent jobs run the tests. The only language input is the
plugin's language token (and the service name the language reads from its
project), so Java and Python later only add a token.
- Declarative: add datadog(testOptimization: …) to the top-level options,
creating the directive after agent or environment, or merge into an
existing options/datadog(…) call (enabled: true, the language added to
languages, every other argument kept, testVisibility edited in place).
- Scripted: wrap each outermost node block that runs the tests in
datadog(testOptimization: …) { … }, indented one step; multi-line
strings keep their bytes.
- Vitest and ESM Mocha: add the DD_TRACE_PATH loader line to a
single-quoted test sh, or ask for it and resume with onboard --apply.
- Refuse, with the instructions and the reason instead: shared libraries,
container-only test jobs, computed arguments, node blocks in def
functions, several datadog calls that differ, and any result PR-A's
checker does not read as a complete setup.
Edits use the parser's byte spans, keep indentation and line endings, and
are re-read before anything is written. JavaScript gets the automatic
setup, asking for the plugin confirmations right after the edit. Ruby gets
--apply on Jenkins only: it writes the Jenkinsfile edit and returns the
datadog-ci pin as agent instructions.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Without a project name, serviceName is the origin remote's repository name, falling back to the checkout directory only without a remote: a CI checkout's directory is often a workspace name, not the repository's. The Ruby handoff to onboard --apply now forwards the same flags as the automatic handoff, and a golden case covers a container node outside the wrapped test node. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
onboard --apply already refused a Jenkinsfile whose test jobs all run in containers. It now refuses any edit that would put a job running steps in a container inside the option's scope (the whole declarative pipeline, or a wrapped node), because the plugin's tracer variables reach those container steps too. The refusal names the jobs and the user's options; onboarding then gives the instructions with that reason. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A value in a pipeline's or stage's environment directive, such as
X = sh(script: 'git show -s --pretty=%an', returnStdout: true).trim() or an
sh in a GString's ${...}, runs when Jenkins enters that scope. The reader
now evaluates it like any other expression (sh, load, build, a Jenkinsfile
function or an unknown step) and records what it runs where Jenkins runs
it: before a stage's steps, in that stage's job, and for the pipeline, a
stage of stages or a matrix cell, in a job of its own named environment or
<stage> (environment). The variable's value stays unknown, and a
credentials() binding is read as before.
The steps see the Datadog plugin's variables of the pipeline's top-level
option, as the plugin sets them for every step. A command that runs no
tests lands in the shared review of non-test jobs, and a test command is
validated.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 32df9cc | Docs | View more details | Give us feedback! |
|
/datadog autotest review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66d1d8281f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| target := ciSetupTarget{Workflow: workflow.Path, Job: name, Step: first} | ||
| if framework == "mocha" && !esm { | ||
| target.After = []string{} | ||
| } |
There was a problem hiding this comment.
When a Jenkins job contains multiple matching sh steps, this creates a target only for the first one. The Jenkins ESM bootstrap in request.After exports NODE_OPTIONS only in that individual shell, so a later Vitest/Playwright test step—or the actual ESM Mocha step when an earlier CommonJS step matched first—runs without the required --import loader even though the resulting setup is accepted as complete. Create targets for every matching test step that needs the shell-local bootstrap.
Useful? React with 👍 / 👎.
| } | ||
| esm[calls[0].call] = target.after | ||
| } | ||
| for _, current := range calls { |
There was a problem hiding this comment.
For a scripted Jenkinsfile with multiple sh calls on one physical line, such as separate node { sh 'npm test' }; node { sh 'deploy' } blocks, calls contains every call on that line and this loop wraps or merges all of their enclosing scopes. For setups without an after command, no ambiguity check runs, so onboard --apply can instrument unrelated deployment or build nodes instead of only the node containing the discovered test command.
Useful? React with 👍 / 👎.
| text, _ := script.Value.literal() | ||
| if !slices.ContainsFunc(commands, func(command string) bool { return !strings.Contains(text, command) }) { | ||
| return nil |
There was a problem hiding this comment.
This treats a required bootstrap command as satisfied when its text occurs anywhere in the sh literal. If the command is after the test command or appears only in a comment, the editor makes no change and subsequently reports the Jenkins setup as complete, although the ESM test ran before NODE_OPTIONS was extended. Check executable ordering relative to each test command, or refuse the edit when that ordering cannot be established.
Useful? React with 👍 / 👎.
![datadog-official[bot]](https://avatars.githubusercontent.com/in/1358509?s=60&v=4){kind=small}
There was a problem hiding this comment.
Two of the most critical issues are: (1) merging named arguments into an existing positional-argument datadog(…) call produces a broken Jenkinsfile that Jenkins rejects but the post-edit checker accepts, and (2) ESM loader injection targets only the first sh call in a multi-shell stage, leaving all subsequent shells uninstrumented despite the checker reporting success. Additionally, overlapping edits between node-block wrapping and loader insertion block automatic setup for scripted Vitest/ESM Mocha pipelines, and --command validation fires only after source files have already been modified.
🤖 Bits Code Review · Commit 66d1d82
| return nil | ||
| } | ||
| e.merged[call] = true | ||
| key, value, ok := jenkinsTestOptimization(call) |
There was a problem hiding this comment.
For an existing scripted wrapper such as datadog([collectLogs: true]) { … }, merging appends testOptimization as a named argument outside the positional map. The resulting call supplies two map arguments, which Jenkins rejects. ddtest's post-edit checker nevertheless accepts it and writes the broken pipeline. Refuse positional/computed arguments or merge inside the existing literal map.
| key, value, ok := jenkinsTestOptimization(call) | |
| if positional, _ := jenkinsArgs(call); len(positional) > 0 { | |
| return e.gap(call, "The datadog(…) call has positional or computed arguments, so ddtest cannot safely merge the setup into it.") | |
| } | |
| key, value, ok := jenkinsTestOptimization(call) |
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
| } | ||
| } | ||
| for _, call := range slices.SortedFunc(maps.Keys(esm), func(a, b *groovyCall) int { return a.Start - b.Start }) { | ||
| if err := e.prepend(call, esm[call]); err != nil { |
There was a problem hiding this comment.
For an unconfigured scripted pipeline running Vitest or ESM Mocha, wrap schedules replacement of the entire node block, then prepend schedules a loader insertion inside that replacement. apply consequently returns “The planned edits overlap,” preventing automatic setup even for a simple single-quoted test script. Compose the loader change into the wrapper replacement or generate nonoverlapping edits.
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
| return nil, &SetupGap{location, err.Error()} | ||
| } | ||
| } | ||
| target := ciSetupTarget{Workflow: workflow.Path, Job: name, Step: first} |
There was a problem hiding this comment.
When a Jenkins job runs Vitest or ESM Mocha in multiple sh calls on separate lines, only its first test shell becomes an edit target. Each shell has its own environment, so later shells miss the ESM loader and run without the required instrumentation. The new setup checker also returns after the first test, allowing this incomplete configuration to be written. Collect and validate every test shell requiring the loader.
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
| _, err = fmt.Fprintln(output, "Preview only; no source files changed and no tests executed. Review this diff, then "+next) | ||
| return err | ||
| } | ||
| changes, err := plan.Apply(apply.AllowDirty) |
There was a problem hiding this comment.
On a Ruby Jenkins project with its CI runtime supplied, onboard --apply --yes --command … writes the Jenkinsfile before StartWithOptions rejects the command override as invalid_options. An invalid request therefore changes setup for all discovered tests; previously it was rejected without editing source. Delegate the rejection before applying the plan.
| changes, err := plan.Apply(apply.AllowDirty) | |
| if options.Command != "" { | |
| return start() | |
| } | |
| changes, err := plan.Apply(apply.AllowDirty) |
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
onboard --agent decided that onboard --apply writes a JavaScript framework's Jenkins plugin setup by planning the edit for any site, so a plugin the user confirmed reports to another Agentless site than DD_SITE (or --site) was handed to onboard --apply, which then refused the edit as incomplete. Plan it for the selected site, DD_SITE by default, as onboard --apply does: a mismatch leaves the setup to the instructions, whose ci_setup_required result names it, as before onboard --apply edited Jenkinsfiles. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
What
ddtest onboard --applynow writes the Datadog Jenkins plugin setup into Jenkinsfiles.datadog(testOptimization: [enabled: true, serviceName: '<project>', languages: ['<LANG>']])to the top-leveloptions. If the pipeline already has anoptionsblock or adatadog(…)call (including the legacytestVisibility:), it merges into it.nodeblock that runs tests indatadog(…) { … }.DD_TRACE_PATH--importline to the testsh, when that script is single-quoted.--yes, the edit with--yes, then the plugin confirmations, then validation.--applyon Jenkins only. ddtest writes the Jenkinsfile edit, and thedatadog-cipin is returned as agent instructions.nodeblocks insidedeffunctions and conflictingdatadog(…)calls are refused. ddtest gives the reason and the manual instructions instead.--applyagain changes nothing.environment {}value runs (X = sh(script: …, returnStdout: true), Jenkinsfile functions, library calls), where Jenkins runs them, instead of reading only the value. Non-test commands there only add review notes.Why
Under #266, every Jenkins user or agent edits the Groovy by hand, which is the step agents get wrong most often. Examples are a stage-level option, a missing
serviceName, or a re-indented string. The plugin setup needs only the language's plugin token, so ddtest can write it safely from the parser's byte spans. It re-reads the result with #266's checker before writing anything. See the plugin docs.E2E testing
Prerequisites: a
ddtestbuilt from this branch (make build), git, Node 22+ with npm, and Ruby 3.2+ with Bundler. For each scenario, copy the fixture into a fresh temp directory, then rungit init && git add -A. Don't commit:--allow-dirtyextends the staged files. No Jenkins is needed.internal/onboard/testdata/jenkins-javascript, thennpm install):ddtest onboard --framework jest --agent --ci-runtime node=22:ready, andnext_commandisonboard … --apply --yes.ddtest onboard --framework jest --apply --ci-runtime node=22: a diff that addsoptions { datadog(testOptimization: [enabled: true, serviceName: 'jenkins-javascript-fixture', languages: ['JAVASCRIPT']]) }afteragent, andPreview only; ….Jenkinsfileis unchanged.--yes --allow-dirty --format agent:ci_setup_confirmation_required, withchangeslistingJenkinsfileand.gitignore.--applyagain:Jest CI setup is already configured; no edits.--apply --yes --allow-dirty --format agent --ci-runtime node=22 --ci-confirm jenkins-plugin --ci-confirm jenkins-plugin-mode=agentless:datadoghq.com:complete_locally.ddtest testdrive --reportwith the same flags showsCOMPLETE LOCALLY.internal/onboard/testdata/jenkins-ruby):ddtest onboard --agent --ci-runtime ruby=3.2:ci_setup_required, withnext_action: run_commandandnext_commandset toonboard --framework rspec --apply --yes ….--allow-dirty.node { … }is now insidedatadog(testOptimization: […, languages: ['RUBY']]) { … }, indented one more step (4 spaces).agent_instructionincludesPin datadog-ci in the bundle, andresume_commandistestdrive … --all.gem "datadog-ci", "1.42.0", group: :testtoGemfile, runbundle lock, and add/.testoptimization/to.gitignore.testdrive --framework rspec --all --agent --yes --ci-runtime ruby=3.2with the two--ci-confirmflags:complete_locally.ddtest onboard --apply --ci-runtime ruby=3.2:RSpec CI setup is already configured; no edits.options { timeout(time: 1, unit: 'HOURS') \n datadog(collectLogs: true) }(one option per line) afteragent. The preview addstestOptimization: [...]to thatdatadog(…)call and keepstimeoutandcollectLogs.datadog(testVisibility: [enabled: false, serviceName: 'scigateway', languages: ['RUBY']]). The preview edits it in place toenabled: trueandlanguages: ['RUBY', 'JAVASCRIPT']."test": "vitest run",vitest@3.2.4and an ESM test, thennpm install):ddtest onboard --framework vitest --apply --ci-runtime node=22. The preview also changessh 'yarn test'tosh 'if [ -n "${DD_TRACE_PATH:-}" ]; then export NODE_OPTIONS="${NODE_OPTIONS:-} --import $DD_TRACE_PATH/register.js"; fi; yarn test'.Jenkinsfileis unchanged.@Library('pipeline-lib') _after the first line.onboard --framework jest --agent --ci-runtime node=22returnsci_setup_requiredwithapply_source_edit, andreasonincludesonboard --apply cannot write this setup (… loads a shared library (@Library) …). Plain--applyprints that refusal.agent { docker { image 'node:22' } }.--apply --yes --format agentreturnsci_setup_unsupported.agent { label 'sl7'}and addscript { docker.image('node:22').inside { sh 'npm pack' } }aftersh 'yarn test'.onboard --framework jest --agent --ci-runtime node=22returnsci_setup_required, andreasonsays the option "would also apply to Jenkinsfile / Unit Tests (docker.image('node:22').inside), which run steps in a container".--apply --yes --allow-dirty --format agentreturnsci_edit_unsupportedwith the same reason.datadog(testOptimization: [enabled: params.TRACE, …]). The result names the computedenabled.🤖 Generated with Claude Code