feat: support environment variables for App (#188) by 1122Louis · Pull Request #284 · SAP/crossplane-provider-cloudfoundry

1122Louis · 2026-05-11T23:18:23Z

Closes #188

Changes
Architecture
Replaced UpdateAndPush with direct CF Environment Variable API (PATCH /v3/apps/:guid/environment_variables) for env var updates: no push, no restage, no unnecessary restart.

Bug fixes
Fixed nil deletion bug: updateEnvVars now calls GET /v3/apps/:guid/environment_variables first, then sends nil for any var present in CF but absent from spec , so removed vars are actually deleted.
Fixed resulting infinite update loop: was caused by removed vars never being cleared from CF, keeping drift alive.
Fixed double manifest parse: DetectChanges now calls getAppManifest once and shares the result across all checks.

Refactoring
Separated docker_image and environment handling in the controller: docker credentials are only fetched when the docker image actually changes.
Added GetEnvironmentVariables and SetEnvironmentVariables to AppClient interface and fake mock.

Tests
Added 5 unit tests covering env var drift detection (app_test.go):
Same in spec and CF → no drift
Different in spec vs CF → drift detected
Set in spec, missing in CF → drift detected
Removed from spec, still in CF → drift detected (the nil deletion bug)
Nil in both → no drift
Added 3 controller TestUpdate cases (controller_test.go):
EnvVarAdded — new var in spec, CF has none → SetEnvironmentVariables called correctly
EnvVarDeleted — var removed from spec, CF still has it → sent as nil to CF
EnvVarGetFails — GetEnvironmentVariables errors → update returns wrapped error

Copilot

Pull request overview

This PR implements functional support for spec.environment on the App managed resource by wiring environment variables into manifest generation, drift detection, and update behavior, addressing #188.

Changes:

Include environment variables when building the Cloud Foundry app manifest for pushes.
Detect environment-variable drift and trigger a re-push when env changes are observed.
Update API comments to reflect that environment variables are now supported.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File	Description
internal/controller/app/controller.go	Triggers `UpdateAndPush` when env drift is detected (in addition to docker image drift).
internal/clients/app/push.go	Adds env var mapping into the CF manifest used for push operations.
internal/clients/app/app.go	Adds env var drift detection by comparing desired env JSON vs generated manifest env.
apis/resources/v1alpha1/app_types.go	Removes “NOT SUPPORTED YET” note from the `Environment` field comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

SatabdiG

We should definitely add test file.

As far as I am aware there are two ways CF handles env vars

The Manifest/Push path - A sledgehammer of sorts, applies a full manifest, res-stages, restarts the app.
The Environment Variable API — CF has a dedicated PUT /v3/apps/:guid/environment_variables endpoint. This is more of a scalpel and doesn't require a full restage of the app.

My core concern and question is that we are reusing the existing pattern for docker image changed -> push, does env variable need a push? It likely does not if we go via the API. Also push are expensive even more for crossplane reconcilers than the plain cf push.

Maybe you could help me through your reasoning here.

SatabdiG

good work on the restructure.

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.

SatabdiG

Nice work but I think it introduced some new bugs

- Replace UpdateAndPush with SetEnvironmentVariables for env var updates - Fix nil deletion bug in envVarsChanged (env vars removed from spec now detected as drift) - Parse AppManifest once in DetectChanges instead of twice - Separate docker_image and environment handling in controller - Add SetEnvironmentVariables to AppClient interface and fake mock - Add 5 env var test cases covering drift detection scenarios

- Extract updateDockerImage and updateEnvVars helpers from Update - Add HasOtherChanges method to ChangeDetection to simplify condition - Fix import grouping in app_test.go (goimports)

When an env var is removed from spec.forProvider.environment, it was not being deleted from CF because SetEnvironmentVariables merges rather than replaces. Fix by calling GetEnvironmentVariables first to get current CF state, then sending nil for any var present in CF but absent from spec. Also adds GetEnvironmentVariables to AppClient interface and mock.

… assert mock expectations

…ng]string

… changed

…siently

1122Louis · 2026-05-20T07:47:58Z

Thanks for the detailed review. Here's what I addressed:

STOPPED app being started / double restart on docker+env change
Fixed in updateEnvVars, it now takes a dockerAlsoChanged bool parameter. If the app is STOPPED or a docker push already happened (which CF uses to restart), the Stop/Start is skipped.
Missing test cases
Added EnvVarUpdated_AppStopped (verifies no Stop/Start when app is STOPPED) and EnvVarAndDockerChanged (verifies no Stop/Start when docker image also changed).
kubebuilder validation for environment field
Changed the type from *runtime.RawExtension to map[string]string. This makes the type itself the validation, controller-gen generates additionalProperties: type: string in the CRD schema, so Kubernetes rejects non-string values at the API level. No explicit marker needed, and it also removes all the json.Unmarshal boilerplate from the controller and push.go.
AppManifest cleared to "" on transient GenerateManifest failure
Fixed in Observe, it saves prevAppManifest before resetting AtProvider, and restores it if GenerateManifest fails, so DetectChanges is never comparing against a stale empty manifest.
nil-panic in SetEnvironmentVariables mock
Added the same nil guard that GetEnvironmentVariables already had.

…t detection, add Stop/Start failure tests

SatabdiG

Very good work, this is almost there. I have just one comment.

SatabdiG

lgtm. Thanks for sticking with this 🥇

Copilot AI review requested due to automatic review settings May 11, 2026 23:18

1122Louis requested a deployment to pr-e2e-approval May 11, 2026 23:18 — with GitHub Actions Waiting

Copilot started reviewing on behalf of 1122Louis May 11, 2026 23:19 View session

Copilot AI reviewed May 11, 2026

View reviewed changes

Comment thread internal/clients/app/push.go

Comment thread internal/clients/app/app.go Outdated

Comment thread internal/clients/app/app.go Outdated

Comment thread internal/clients/app/app.go Outdated

1122Louis requested a deployment to pr-e2e-approval May 11, 2026 23:24 — with GitHub Actions Waiting

1122Louis requested a deployment to pr-e2e-approval May 11, 2026 23:31 — with GitHub Actions Waiting

SatabdiG requested changes May 12, 2026

View reviewed changes

Comment thread internal/clients/app/app.go Outdated

Comment thread internal/clients/app/app.go Outdated

Comment thread internal/controller/app/controller.go Outdated

SatabdiG assigned SatabdiG and 1122Louis and unassigned SatabdiG May 12, 2026

1122Louis requested a deployment to pr-e2e-approval May 12, 2026 14:30 — with GitHub Actions Waiting

1122Louis requested a deployment to pr-e2e-approval May 12, 2026 14:36 — with GitHub Actions Waiting

1122Louis requested a deployment to pr-e2e-approval May 12, 2026 14:55 — with GitHub Actions Waiting

1122Louis requested a review from SatabdiG May 12, 2026 19:40

1122Louis requested a deployment to pr-e2e-approval May 14, 2026 15:25 — with GitHub Actions Waiting

1122Louis temporarily deployed to pr-e2e-approval May 14, 2026 16:48 — with GitHub Actions Inactive

SatabdiG reviewed May 19, 2026

View reviewed changes

Comment thread internal/clients/app/push.go

Comment thread internal/controller/app/controller.go

Comment thread internal/controller/app/controller_test.go

Comment thread internal/controller/app/controller_test.go

1122Louis requested a deployment to pr-e2e-approval May 19, 2026 11:13 — with GitHub Actions Waiting

1122Louis added 3 commits May 19, 2026 13:16

feat: support environment variables for App (SAP#188)

301376f

fix: reduce cyclomatic complexity in DetectChanges

a971ad6

chore: regenerate CRDs after app environment field update

ccfe75d

1122Louis requested a deployment to pr-e2e-approval May 19, 2026 11:17 — with GitHub Actions Waiting

1122Louis requested review from SatabdiG and Copilot May 19, 2026 12:46

Copilot started reviewing on behalf of 1122Louis May 19, 2026 12:46 View session

Copilot AI reviewed May 19, 2026

View reviewed changes

Comment thread internal/controller/app/controller.go Outdated

Comment thread internal/clients/app/app.go

Comment thread internal/controller/app/controller.go Outdated

Comment thread internal/clients/app/push.go Outdated

Comment thread internal/clients/fake/app.go

SatabdiG requested changes May 19, 2026

View reviewed changes

1122Louis added 3 commits May 19, 2026 16:43

fix: reduce cyclomatic complexity and fix import grouping

ceced4b

- Extract updateDockerImage and updateEnvVars helpers from Update - Add HasOtherChanges method to ChangeDetection to simplify condition - Fix import grouping in app_test.go (goimports)

1122Louis added 5 commits May 19, 2026 16:43

test: add TestUpdate cases for env var add/delete/error

133e1bd

fix: restore LogRateLimitPerSecond, restart app after env var update,…

c55b65a

… assert mock expectations

refactor: change Environment field type from RawExtension to map[stri…

0fa02c0

…ng]string

fix: skip restart in updateEnvVars when app is stopped or docker also…

3bf885b

… changed

fix: preserve AppManifest in Observe when GenerateManifest fails tran…

e0672b9

…siently

1122Louis force-pushed the feature/188-app-env-variables branch from da23538 to e0672b9 Compare May 19, 2026 19:01

1122Louis requested a deployment to pr-e2e-approval May 19, 2026 19:01 — with GitHub Actions Waiting

fix: correct import grouping in controller_test.go

2a0094d

1122Louis requested a deployment to pr-e2e-approval May 19, 2026 19:24 — with GitHub Actions Waiting

chore: regenerate deepcopy and CRD after Environment type change

d5d736a

1122Louis temporarily deployed to pr-e2e-approval May 19, 2026 19:46 — with GitHub Actions Inactive

1122Louis requested a review from SatabdiG May 20, 2026 07:51

SatabdiG requested changes May 20, 2026

View reviewed changes

Comment thread internal/clients/app/app.go Outdated

Comment thread internal/controller/app/controller_test.go

Comment thread internal/controller/app/controller_test.go

fix: address review — fail Observe on manifest error, fix docker drif…

9b6cdb5

…t detection, add Stop/Start failure tests

1122Louis requested a deployment to pr-e2e-approval May 26, 2026 14:46 — with GitHub Actions Waiting

SatabdiG reviewed May 27, 2026

View reviewed changes

Comment thread internal/clients/app/app.go Outdated

refactor: remove redundant GetEnvironmentVariables wrapper

5a0334f

1122Louis requested a deployment to pr-e2e-approval May 27, 2026 12:03 — with GitHub Actions Waiting

Merge branch 'main' into feature/188-app-env-variables

89325b5

1122Louis temporarily deployed to pr-e2e-approval May 27, 2026 12:04 — with GitHub Actions Inactive

SatabdiG approved these changes May 27, 2026

View reviewed changes

Merge branch 'main' into feature/188-app-env-variables

3410397

1122Louis temporarily deployed to pr-e2e-approval May 27, 2026 14:12 — with GitHub Actions Inactive

SatabdiG merged commit f1e151f into SAP:main May 28, 2026
12 checks passed

Conversation

1122Louis commented May 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SatabdiG left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SatabdiG left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SatabdiG left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

1122Louis commented May 20, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SatabdiG left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SatabdiG left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

1122Louis commented May 11, 2026 •

edited

Loading