Ma77Ball commented on code in PR #8546:
URL: https://github.com/apache/texera/pull/8546#discussion_r4030756642
##########
computing-unit-managing-service/src/test/scala/org/apache/texera/service/resource/CuratedImageResourceSpec.scala:
##########
@@ -301,12 +299,10 @@ class CuratedImageResourceSpec extends AnyFlatSpec with
Matchers {
"sha256:1111111111111111111111111111111111111111111111111111111111111111"
}
- "the feature flag" should "be off unless a deployment turns it on" in {
- CuratedImageConfig.enabled shouldBe false
- }
-
- it should "start no unit from a curated image while it is off" in {
- CuratedImageResource.readyImageFor(1) shouldBe None
+ // On by default now that the pages to manage and choose images have
shipped. A
+ // deployment short of node disk, or unwilling to offer them, sets it back
to false.
+ "the feature flag" should "be on unless a deployment turns it off" in {
Review Comment:
Adapting the flag-value assertion is right, but this drops the only test of
the disabled-path guard. The old `readyImageFor(1) shouldBe None` case (removed
with this block) was what exercised `readyImageFor` returning `None` when the
feature is off.
Why it matters: `readyImageFor` still short-circuits to `None` when
`CuratedImageConfig.enabled` is false (line 187), a safety guard so a
deployment that turned the feature off, or left a row behind from when it was
on, never starts a unit from a curated image. With the flag now on by default
and that test gone, nothing exercises the off-path; a future refactor could
delete the guard and the suite would stay green.
Suggested fix: no clean one-click edit (the config is read at load, so the
flag has to be forced off). Re-add an off-path case that drives `readyImageFor`
with the feature disabled, e.g. via the `TEXERA_CURATED_IMAGES_ENABLED` env
hook the config already reads, and assert `None`.
##########
frontend/src/app/common/component/computing-unit-create-modal/computing-unit-create-modal.component.ts:
##########
@@ -171,15 +182,36 @@ export class ComputingUnitCreateModalComponent implements
OnInit, OnChanges {
ngOnChanges(changes: SimpleChanges): void {
if (changes["visible"]?.currentValue === true) {
this.resetAdvancedSettings();
+ this.loadCuratedImages();
}
}
+ /**
+ * Read when the dialog opens rather than in ngOnInit: both hosts render
this component
+ * unconditionally, so ngOnInit runs once at page load. An image that became
ready since
+ * then would never appear, and one failed read would hide the field for the
session.
+ *
+ * Readable by any signed-in user. A deployment with curated images off
answers 503, and
+ * a user who never sees the dropdown gets exactly today's behaviour.
+ */
+ private loadCuratedImages(): void {
+ this.cuImageService
+ .list()
+ .pipe(untilDestroyed(this))
+ .subscribe({
+ next: images => (this.curatedImages = images.filter(isStartable)),
+ error: () => (this.curatedImages = []),
Review Comment:
This collapses every error from `list()` to "no images", not just the 503
that means the feature is off.
Why it matters: now that the feature is enabled by default the endpoint
returns real data, so a transient 500 or network blip silently hides the picker
and the unit falls back to the deployment image with no feedback. A user who
just registered an image and expects to choose it simply sees the field
missing. The admin page's `load()` already distinguishes a 503 from a real
error and surfaces the latter; this path does not.
Suggested fix (optional, this is a nit): treat only `HttpErrorResponse`
status 503 as feature-off (set to `[]`) and otherwise keep the field absent
while letting a real error be visible, mirroring the admin component. If the
silent fail-safe-to-default is intentional, leave it as is.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
