Add dependabot smoke test reminder comment - #3
Draft
ajalon1 wants to merge 1 commit into
Draft
Conversation
Dependabot PRs run with Secret source: Dependabot, which restricts access to repository secrets (including DR_API_TOKEN). This causes smoke tests to silently fail when auto-triggered by the go label. A maintainer must manually add the run-smoke-tests label to get a passing run. This job posts a comment on every dependabot PR so that fact is visible at review time rather than discovered at merge time. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ajalon1
force-pushed
the
claudy/wizardly-noether-4a8db6
branch
from
May 11, 2026 21:40
c4f2463 to
5c9daba
Compare
ajalon1
pushed a commit
that referenced
this pull request
Sep 11, 2026
…datarobot-oss#906) * feat(pipeline): remove feature gate for GA and publish docs `dr pipeline` was dropped from the command tree, shell completion and `dr --help` unless DATAROBOT_CLI_FEATURE_PIPELINE=true was exported. It is now registered unconditionally and the Private Preview note is gone from its help text. The gate mechanism itself is untouched: `dr workload` and `dr artifact` still use it. - Drop features.SetGate(cmd, "pipeline") and the now-unused import. - Invert TestCmd_FeatureGate into TestCmd_NotFeatureGated, and add TestPipelineCommandPresentByDefault as the end-to-end guard that cli.CommandAdder no longer filters the command out. - Add the 38 already-telemetry-wired `dr pipeline ...` paths to expectedTrackedCommands. They are reachable from the live RootCmd now, so unlike workload/artifact they need no separate standalone list. - Close a pre-existing gap in TestCmd_HasExpectedSubcommands, which asserted 11 of the 15 registered subcommands. Docs: commands/pipeline.md and commands/pipelines-reference.md already existed but were absent from the mkdocs nav, so the only way to reach them was to already know the URL. Both are now listed under Commands, and no longer open by telling the reader to turn a feature flag on. The command tree also documented a `dr pipeline environment` subcommand that has never existed under that name (the code registers `image`) and omitted `clone` and `source` entirely; all three are corrected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(pipeline): list the run task subgroup in the command tree The GA commit corrected the pipeline command tree in docs/commands/README.md (environment→image, added clone and source) but left the `run` block ending at `cancel`, omitting the `run task` subgroup. Those commands (`run task list/get/logs/result`) exist, are telemetry-wired, and are already documented in pipeline.md and pipelines-reference.md, so the tree was the only place under-listing the live command surface. Add the subgroup to the ASCII tree and mention it in the detailed `run` bullet. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(pipeline): percent-encode resource ids in every client URL builder Pre-GA security review (QA-14319). Caller-supplied ids were concatenated straight into request paths, so an id containing "/", ".." or "?" could rewrite the authenticated request: "../p-1" is normalized by routers and proxies into a different /api/v2 route, and "?" moves the rest of the path into the query string. Every id now goes through the package's existing escapeID (url.PathEscape), matching what internal/workload already does at every call site. The review listed 8 sites; there were 19. The ones it did not name: * version.go:76,106 -- pipelineID in the /versions routes * schedule.go -- scheduleID appended in get/update/delete * run.go:130,148,166 -- runID interpolated into the PipelinePath suffix * input.go:110,128,147 -- inputID, same * run_task.go -- taskBase takes TWO ids; only one was mentioned The suffix cases are the reason PipelinePath does not escape its suffix argument and now documents that: the suffix legitimately contains "/" as a separator, so escaping it wholesale breaks the path, and escaping it per-segment would still let an id containing "/" split into two segments. Ids interpolated into a suffix must therefore be wrapped by the caller. Tests: path_escaping_test.go drives every builder with the id "a/b?c/../x" and asserts against r.RequestURI -- the raw target, i.e. what the client actually sent. It deliberately does not assert on r.URL.Path: that field is the decoded path, where a correctly-encoded %2F reads back as a plain "/", so the assertion would fail on working code. Routers and path-normalizers dispatch on the raw target, so that is where the property lives. Verified the guard bites: reverting the scope.go and image.go escapes fails 11 subtests. GetPipeline/DeletePipeline are included even though they were already escaped, so they stay that way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(config): stop dumping request bodies to the debug log Pre-GA security review (QA-14319). With --debug, every request body was written to stderr and .dr-tui-debug.log via config.RedactedReqInfo. The redactor is name-keyed and masks only apiToken|password|passwd|secret| privateKey|clientSecret|refreshToken|accessToken|token. That assumption holds for DataRobot's own endpoints, where the schema is known. It does not hold for `dr pipeline input create`, whose body is arbitrary user-authored JSON -- and an input set is the natural place to put data-source credentials. A secret under any other key (api_key, credentials, db_password) landed in the log in the clear, permanently. Taking the option the ticket recommended: RedactedReqInfo now passes false to httputil.DumpRequestOut, so it renders method, URL and headers only, with Authorization still masked. A body that is never logged cannot be under-redacted, which is a stronger guarantee than extending the regexp to names we happen to think of. Second benefit, as the ticket predicted: DumpRequestOut(req, true) *drained* req.Body, so callers had to re-arm it before sending or the server got an empty payload. A headers-only dump never touches Body, so this deletes: * the restoreRequestBody call and helper in drapi/post.go * the same call in drapi/patch.go and drapi/delete.go * the log-level guard in pipeline.go, which existed only because the body dump silently broke multipart uploads (ContentLength=N with a 0-byte body) RedactSecretFields is kept and still applied: a secret can appear in a non-Authorization header, and it is exported. Its comment no longer claims the dump includes bodies. Tests: TestRedactedReqInfo_OmitsRequestBody asserts a body carrying api_key/db_password reaches the log in no form at all -- not even the non-secret field, since the body is omitted rather than filtered -- while method, URL and Content-Type survive and Authorization is masked. TestRedactedReqInfo_DoesNotDrainBody pins the body still being readable after logging, which is what makes the drapi cleanup safe. Both fail if the dump is switched back to true. Existing redaction tests unchanged and passing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(pipeline): correct the schedule contract to match the live API Pre-GA review (QA-14319) flagged a divergence between the reference and the implementation and asked for a product call on which was right. No call needed -- the API answered. Probed against staging with a real locked pipeline that has a schedule: GET /pipelines/{id}/schedules -> 200 GET /pipelines/{id}/schedules/{sid} -> 200 GET /pipelines/{id}/versions/1/schedules -> 404 {"detail":"Not Found"} GET /pipelines/{id}/versions/1/schedules/{s} -> 404 {"detail":"Not Found"} The code was right; the reference documented routes that do not exist. Schedules hang off the pipeline, and the version travels in the create body as pipeline_version_id. Three errors fixed, not one: 1. Route shapes: /pipelines/{id}/versions/{ver}/schedules… -> /pipelines/{id}/schedules… 2. `--version` was listed as required on list/get/update/delete. Those commands have no such flag, so the documented invocations could not run -- and not with a "required flag not set" error either, just an unknown flag. 3. `create` was missing `--image` and `--image-version`, both of which are required (cmd/pipeline/schedule/create/cmd.go:93,95). Every documented `create` example would have failed. Also corrected the "every verb requires both --pipeline and --version" intro in pipelines-reference.md and the matching sentence in pipeline.md:43. A schedule does still target a locked version, and the version comes back on reads -- it is just not part of the URL. Flags checked against the cobra definitions rather than transcribed: list also takes --offset/--limit (default 100), and update requires at least one of --cron/--timezone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(pipeline): fix reference gaps from CMPT-7810 GA review Three doc corrections AJ Alon found in the pre-GA reference review (CMPT-7810). Item 2 (schedule-create contract) was already fixed in 3ffd31b; these close the other three. - run create: `--image` is required in code (run/create/cmd.go marks it MarkFlagRequired), but the reference called it optional and two of the three examples omitted it, so following them failed with `required flag(s) "image" not set`. Mark it required and add `--image` to every example. - image version logs: the command exists (`GET /pipelines/images/{id}/versions/{n}/logs`) but was absent from the Images table and the quick endpoint lookup, leaving the reference inconsistent with the README command tree. Add both. - README command tree: add the `pipelines` alias to the pipeline entry, matching how sibling commands (plugin, llm-gateway) list theirs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(pipeline): add image get to the reference (same gap class as CMPT-7810 #3) `dr pipeline image get` (`GET /pipelines/images/{id}`) was missing from the Images table and the quick endpoint lookup, the same inconsistency CMPT-7810 item 3 flagged for `image version logs`. Add both rows so the reference matches the README command tree and the live command surface. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Hanu <hanumayil@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
RATIONALE
Dependabot PRs consistently fail their first smoke test run. After investigation, the root cause is GitHub's secret isolation model: any workflow triggered on a dependabot-created PR runs with
Secret source: Dependabot, which means only Dependabot-specific secrets are available. Repository secrets likeDR_API_TOKENare not injected. The smoke tests build fine but fail immediately at authentication.A human maintainer adding the
run-smoke-testslabel upgrades the context toSecret source: Actions(full secrets), which is why the manual-label path works.CHANGES
Adds a
dependabot-reminderjob tochecks.yamlthat fires on any PR wheregithub.actor == 'dependabot[bot]'and posts a comment explaining:run-smoke-testslabel before mergingThe comment appears immediately when the PR opens, so reviewers see it before they start reviewing rather than discovering the constraint at merge time.
NOTES
needs:dependency so it posts as early as possibleGITHUB_TOKEN(comment appears fromgithub-actions[bot])DR_API_TOKENas a Dependabot secret in repo settings, which would allow fully automated smoke tests on these PRs