squakez commented on code in PR #5157:
URL: https://github.com/apache/camel-k/pull/5157#discussion_r1490760964
##########
e2e/builder/build_test.go:
##########
@@ -130,11 +131,34 @@ func TestKitMaxBuildLimit(t *testing.T) {
// verify that all builds are successful
Eventually(BuildPhase(ns, buildA),
TestTimeoutLong).Should(Equal(v1.BuildPhaseSucceeded))
+ Eventually(BuildConditions(ns, buildA),
TestTimeoutLong).ShouldNot(BeNil())
Review Comment:
I don't think it's really required a check on this condition, as it will
eventually get scheduled. What may be more interesting is to check that a build
which is supposed to wait, have this condition turned on, and later move to
scheduled.
##########
pkg/controller/build/build_monitor.go:
##########
@@ -56,6 +57,16 @@ func (bm *Monitor) canSchedule(ctx context.Context, c
ctrl.Reader, build *v1.Bui
if runningBuildsTotal >= bm.maxRunningBuilds {
Log.WithValues("request-namespace", requestNamespace,
"request-name", requestName, "max-running-builds-limit", runningBuildsTotal).
ForBuild(build).Infof("Maximum number of running builds
(%d) exceeded - the build (%s) gets enqueued", runningBuildsTotal, build.Name)
+ build.Status.SetCondition(
+ v1.BuildConditionScheduled,
+ corev1.ConditionFalse,
+ v1.BuildConditionWaitingReason,
+ fmt.Sprintf(
+ "Maximum number of running builds (%d) exceeded
- the build (%s) gets enqueued",
Review Comment:
Let's put this into a variable and reuse in the 2 places instead.
##########
pkg/controller/build/schedule.go:
##########
@@ -63,7 +65,7 @@ func (action *scheduleAction) Handle(ctx context.Context,
build *v1.Build) (*v1.
return nil, err
} else if !allowed {
// Build not allowed at this state (probably max running builds
limit exceeded) - let's requeue the build
- return nil, nil
+ return build, nil
Review Comment:
Reading the code comment below, it seems we must store the status of the
build into this critical section, instead of letting it to be done by the
controller. We may extend the `toPendingPhase` to store any generic condition
and state before returning to the controller in general.
##########
pkg/controller/build/build_monitor.go:
##########
@@ -117,6 +128,16 @@ func (bm *Monitor) canSchedule(ctx context.Context, c
ctrl.Reader, build *v1.Bui
if !allowed {
Log.WithValues("request-namespace", requestNamespace,
"request-name", requestName, "order-strategy", bm.buildOrderStrategy).
ForBuild(build).Infof("%s - the build (%s) gets
enqueued", reason, build.Name)
+ build.Status.SetCondition(
Review Comment:
ditto
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@camel.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
