Repository navigation
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
5503744 to
8d412aa
Compare
|
/kind feature |
`shp build upload` created its own BuildRun and exposed only a fixed set of BuildRun fields, so a Local-source build could not request step compute resources and could not be streamed into a BuildRun created elsewhere. This closes the gap flagged when migrating OpenShift BuildConfigs (with spec.resources) to Shipwright. - --step-resources (repeatable): sets BuildRunSpec.StepResources as <step>=<limits|requests>.<resource>=<quantity>, e.g. build-and-push=limits.memory=2Gi. Wired through BuildRunSpecFromFlags, so it is also available on `build run` and `buildrun create`. - --buildrun-name: streams the local source into an existing BuildRun instead of creating one, covering any BuildRun field the CLI does not expose as a flag. Guarded against combination with creation-only flags and against source-bundle builds. Adds unit tests for the flag parser and upload flow, plus e2e cases, and regenerates the command docs. Closes: shipwright-io#415 Co-authored-by: Claude Signed-off-by: Prateek Singh Rathore <prateek.singh.rathore@gmail.com>
8d412aa to
07e9276
Compare
Testing `shp build upload --buildrun-name` on a cluster showed four ways it failed. The flag check rejected global flags, so --namespace and --kubeconfig could not be used with --buildrun-name. The check now looks only at the upload command's own flags. The pod of an existing BuildRun is often already running when the upload starts. The upload only reacted to changes in the pod, and a waiting pod does not change, so it sat idle until the pod's upload waiter gave up and then failed with "container not found". It now also reacts to the first event it sees for the pod. A BuildRun of another Build made the command wait forever, because the upload finds the build pod by the Build name. A BuildRun without a Local source failed with an unclear error about a missing container, because its pod has nothing waiting for the upload. Both are now checked before the upload starts, and the error names the BuildRun. Unit tests cover each case and fail on the code before this commit. The e2e case now waits for the pod to be running before it uploads, passes --namespace, and checks the memory limit on the step container. Co-Authored-By: Claude Signed-off-by: Prateek Singh Rathore <prateek.singh.rathore@gmail.com>
|
Thanks for the review, @sayan-biswas. Testing your two questions turned up four bugs in
I ran every case on a kind cluster and on OpenShift 4.20 with Builds for Red Hat OpenShift 1.9.0. Each case fails on 07e9276 and works on aa15a22. The new unit tests fail on the old code and pass on the new. |
|
/lgtm |
Changes
Closes #415.
shp build uploadcreated its own BuildRun and exposed only a fixed set of BuildRun fields as flags. A Local-source build could not ask for per-step compute resources, and a local folder could not be streamed into a BuildRun created some other way. This came up while migrating OpenShift BuildConfigs that setspec.resourcesto Shipwright with the crane plugin.This PR adds both flags proposed in the issue.
--step-resourcesis repeatable and setsBuildRunSpec.StepResources, in the form<step>=<limits|requests>.<resource>=<quantity>, for examplebuild-and-push=limits.memory=2Gi. Entries for the same step merge. It goes throughBuildRunSpecFromFlags, soshp build runandshp buildrun createget it too.--buildrun-namestreams the local source into an existing BuildRun instead of creating one. That covers any BuildRun field the CLI has no flag for.--buildrun-namefails early, with an error, in these cases.--sa-name. That flag would have no effect. Global flags such as--namespaceare still accepted.The pod of an existing BuildRun is often already running when the upload starts. The upload reacts to the first event it sees for the pod, so it streams at once instead of waiting until the BuildRun times out.
The PR has two commits. 07e9276 adds the two flags. aa15a22 fixes four ways
--buildrun-namefailed when tested on a cluster: it rejected--namespace, it sat idle when the BuildRun's pod was already running, it waited forever on a BuildRun of another Build, and it gave an unclear error on a BuildRun without a Local source.Checks run locally:
go build ./...,make test-unitandmake verify-docspass. The new unit tests fail on 07e9276 and pass on aa15a22.Each case above was also run on two clusters, a kind cluster running Shipwright Build and an OpenShift 4.20 cluster with Builds for Red Hat OpenShift 1.9.0. On both, 07e9276 showed all four failures. With aa15a22 the upload streams through
--namespaceand into a BuildRun whose pod is already running, and both builds succeed. The other cases stop at once with an error that names the problem. A plain upload without--buildrun-namestill builds.Submitter Checklist
See the contributor guide for details on coding conventions, github and prow interactions, and the code review process.
Release Notes
Co-Authored-By: Claude