Mark jumpstarter-client-secret as optional #16
Conversation
WalkthroughThis change updates several Tekton Task YAML files by marking the Changes
Sequence Diagram(s)sequenceDiagram
participant Pipeline
participant RunCmd Task
Pipeline->>RunCmd Task: Start task
alt source workspace is bound
RunCmd Task->>RunCmd Task: cd /workspace/source
else source workspace not bound
RunCmd Task->>RunCmd Task: Skip cd
end
RunCmd Task->>Pipeline: Complete task
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tasks/run-cmd/run-cmd.yaml (1)
68-70: Good defensive check, but quote the boolean for POSIX portability
[ "$(workspaces.source.bound)" == "true" ]relies on the Bash==operator. Using the POSIX-portable single=avoids surprises on/bin/shimages:-if [ "$(workspaces.source.bound)" == "true" ] ; then +if [ "$(workspaces.source.bound)" = "true" ] ; thenMinor, but keeps the script working if the base image is ever swapped to dash or busybox.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tasks/get-lease/get-lease.yaml(1 hunks)tasks/release-lease/release-lease.yaml(1 hunks)tasks/run-cmd/run-cmd.yaml(2 hunks)tasks/setup-sa-client/setup-sa-client.yaml(1 hunks)
🔇 Additional comments (3)
tasks/setup-sa-client/setup-sa-client.yaml (1)
41-41:<<-EOFis OK, but verify that tab-stripping is really required
Usingcat <<-EOFcauses leading tab characters to be stripped from the heredoc. If the heredoc body is indented with spaces (as here), the dash is unnecessary and may surprise future editors. Consider dropping the dash unless you intentionally rely on tab stripping.tasks/get-lease/get-lease.yaml (1)
80-80: Marking the workspace optional can breakjmp config client usewhen no fallback config is provided
When thejumpstarter-client-secretworkspace is omitted, the directory/root/.config/jumpstarter/clientsis no longer a mounted secret but a plain empty dir in the container. If callers also omit theclient-configparam,jmp config client use $(params.client-name)will fail because no client file exists.
Consider one of:
- Add a guard that errors out early when the workspace is un-bound and
client-configis empty.- Provide a sensible default client config baked into the image.
tasks/run-cmd/run-cmd.yaml (1)
82-86: Workspace optionality looks correct—thanks for the bound check
Marking bothjumpstarter-client-secretandsourceas optional is safe now that the script guards against an unbound source workspace. Nice improvement.
| - mountPath: /root/.config/jumpstarter/clients | ||
| name: jumpstarter-client-secret | ||
|
|
||
| optional: true |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same caveat as in get-lease: releasing without a mounted client may fail
With optional: true, the task now starts even when the jumpstarter-client-secret workspace is missing. If the client-config param is also empty, the later jmp config client use … will not find a config and the lease-release will error. Add a safety check or document the expectation in the Task description.
🤖 Prompt for AI Agents
In tasks/release-lease/release-lease.yaml at line 63, the use of 'optional:
true' allows the task to start without the 'jumpstarter-client-secret'
workspace, which can cause errors later if 'client-config' is also empty. To fix
this, add a conditional check before running 'jmp config client use' to ensure
the client config is present, or update the Task description to clearly document
that the workspace or client-config must be provided to avoid lease-release
failures.
|
@NickCao could you replay the patch here? https://github.com/tektoncd/catalog/tree/main/task/jumpstarter-get-lease And, @bkhizgiy we are missing the setup sa client in there. Can we create that task too? |
|
@mangelajo Sure, the PR was merged, but I can open another one. I intentionally removed it because I understood in one of the meetings we had it was going to be sort of a temporary workaround, so do we want to include this anyway? |
|
This is used as part of a konflux pipeline, where you cannot customize the pipelinerun, thus unable to use tekton workspaces, and passing the config-file parameter was the only way to inject the secrets. So while it is a workaround, it could be useful for certain users. |
3 participants
Summary by CodeRabbit
New Features
Style