-
Notifications
You must be signed in to change notification settings - Fork 29
Add --node-selector
Flag for shp build/buildrun
Commands
#309
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 --node-selector
Flag for shp build/buildrun
Commands
#309
Conversation
cc: @adambkaplan sorry for the delay |
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.
Great work so far!
I added my review of the items that appear directly related to the node selector change. The bulk of my comments are related to changing the name of the flag to --node-selector
. Otherwise this is in very good shape. Once the v1beta1
API PR is merged this can be rebased + unblocked.
/ok-to-test |
As requested, a new bats test is added for Context: since Doubt: when running successively the following # create mybuild with pre-defined nodeSelector
shp build create mybuild --node-selector="foo=bar" --source-git-url=https://github.com/shipwright-io/sample-go --output-image=my-fake-image
# run mybuild
shp build run mybuild Outcome: apiVersion: shipwright.io/v1beta1
kind: BuildRun
metadata:
creationTimestamp: "2025-04-23T14:40:59Z"
generateName: mybuild-
generation: 1
labels:
build.shipwright.io/generation: "1"
build.shipwright.io/name: mybuild
name: mybuild-4blqb
namespace: default
resourceVersion: "510650"
uid: 2d74cd53-88fd-469c-a44a-1a5d744027f5
spec: # missing .nodeSelector
build:
name: mybuild
status:
buildSpec:
nodeSelector: # present in status
foo: bar
output:
image: my-fake-image
# ... Expectation:
In case of A, we should consider allowing |
4fb92dc
to
37984fa
Compare
This is expected behavior. Our API has the following convention for settings that are "shared" across
The |
|
Thank you for the clarification @adambkaplan Everything is sorted on my side. Waiting for #304 to be merged to proceed further. |
37984fa
to
c73dfc3
Compare
@adambkaplan @SaschaSchwarze0 I have rebased to the latest commit which includes v1beta1. Please trigger the /ok-to-test . Note: this blocks PR 311 The checks are green on my forked repository. |
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.
/approve
Code changes itself look good! There are only two items IMO that need to be fixed before this merges:
- Revert the changes in the vendor directory (unclear where this newline change came from).
- Squash commits.
vendor/github.com/shipwright-io/build/pkg/apis/build/v1alpha1/build_types.go
Outdated
Show resolved
Hide resolved
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adambkaplan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
- test: add bats for `shp build run` - test: add unit tests Signed-off-by: rxinui <[email protected]>
c73dfc3
to
3027458
Compare
@adambkaplan ready to be merged, commit is squashed. |
--node-selector
Flag for shp build/buildrun
Commands
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.
/lgtm
Looks good!
Changes
Fixes #260
Add CLI flag
--node
to set.spec.nodeSelector
to aBuild
orBuildRun
❗ IMPORTANT: depends on PR 304
PR's codebase is tracking PR 304 by @karanibm6 as
.spec.nodeSelector
is only defined onbuilds.shipwright.io/v1beta1
as a consequence of the currentmain
branch for CLI relying onbuilds.shipwright.io/v1alpha1
(see PR 1683)Submitter Checklist
See the contributor guide
for details on coding conventions, github and prow interactions, and the code review process.
Release Notes