feat(app): add lifecycle_type support for app resource and datasources - #579
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds lifecycle_type support across the Cloud Foundry app resource and app data sources, wiring the value through manifest mapping and exposing it for read parity, along with minimal validation to keep docker_image and lifecycle_type consistent.
Changes:
- Add
lifecycle_typetocloudfoundry_appschema, including config validation for docker lifecycle vsdocker_image. - Expose
lifecycle_typeincloudfoundry_appandcloudfoundry_appsdata sources and map it from CF app/manifest values. - Update generated docs to document the new
lifecycle_typeattribute behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/resources/app.md | Documents lifecycle_type on the app resource. |
| docs/data-sources/apps.md | Documents lifecycle_type on the apps data source. |
| docs/data-sources/app.md | Documents lifecycle_type on the app data source. |
| cloudfoundry/provider/types_app.go | Adds lifecycle_type to TF types and maps lifecycle to/from manifest/app values. |
| cloudfoundry/provider/resource_app.go | Adds lifecycle_type schema + docker consistency validation. |
| cloudfoundry/provider/datasource_apps.go | Adds lifecycle_type to apps list schema output. |
| cloudfoundry/provider/datasource_app.go | Adds lifecycle_type to single app data source schema output. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Adds an optional/computed lifecycle_type attribute to the cloudfoundry_app resource and exposes it in the cloudfoundry_app and cloudfoundry_apps data sources. Wires lifecycle_type into app manifest push/read mapping, applies minimal docker lifecycle consistency validation with docker_image, and updates documentation to describe lifecycle support while allowing additional lifecycle identifiers supported by the target Cloud Foundry platform. Co-authored-by: Gmllt <gilles.miraillet@orange.com>
3c13dea to
0b16d66
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (2)
cloudfoundry/provider/resource_app.go:365
- The new lifecycle_type behavior and ValidateConfig rules (docker_image vs lifecycle_type) are not covered by existing tests in this package (no *_test.go assertions mention lifecycle_type). Adding unit tests for the happy path and the two invalid combinations would help prevent regressions, and adding coverage for both cloudfoundry_app and the app data sources would confirm the new read parity.
if !config.Lifecycle.IsNull() && !config.Lifecycle.IsUnknown() && config.Lifecycle.ValueString() == string(cfv3operation.Docker) &&
!config.DockerImage.IsUnknown() && config.DockerImage.IsNull() {
resp.Diagnostics.AddAttributeError(
path.Root("docker_image"),
"Missing docker image",
cloudfoundry/provider/types_app.go:401
- mapLifecycleToStringType currently prefers app.Lifecycle.Type over the generated manifest's lifecycle field. That can diverge from the PR goal of reading lifecycle configuration from the manifest mapping (and makes it harder to preserve a "missing" lifecycle when the app API always returns a default). Consider preferring appManifest.Lifecycle first (with nil checks) and only falling back to app.Lifecycle.Type when the manifest does not provide a value.
func mapLifecycleToStringType(appManifest *cfv3operation.AppManifest, app *cfv3resource.App) types.String {
if app.Lifecycle.Type != "" {
return types.StringValue(app.Lifecycle.Type)
}
if appManifest.Lifecycle != "" {
Removes duplicated null/unknown guard conditions flagged as new-code duplication in ValidateConfig by factoring them into isDockerLifecycle and hasDockerImage booleans.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (4)
cloudfoundry/provider/types_app.go:398
- mapLifecycleToStringType currently prefers app.Lifecycle.Type over the generated manifest's lifecycle. Since app.Lifecycle.Type is typically always populated by the CF API, lifecycle_type will almost never be null in state, which conflicts with the PR goal of keeping lifecycle_type explicitly null when lifecycle is absent from the manifest mapping. Prefer reading from the manifest first (and return null if the manifest omits it).
if app.Lifecycle.Type != "" {
return types.StringValue(app.Lifecycle.Type)
}
if appManifest.Lifecycle != "" {
return types.StringValue(string(appManifest.Lifecycle))
cloudfoundry/provider/resource_app.go:357
- ValidateConfig introduces new lifecycle_type/docker_image validation behavior, but there are no unit tests covering the valid/invalid combinations (e.g., lifecycle_type="docker" without docker_image, docker_image with lifecycle_type set to a non-docker value, omitted lifecycle_type with docker_image). Adding test cases in the existing app resource tests would prevent regressions.
func (r *appResource) ValidateConfig(ctx context.Context, req resource.ValidateConfigRequest, resp *resource.ValidateConfigResponse) {
var config AppType
resp.Diagnostics.Append(req.Config.Get(ctx, &config)...)
if resp.Diagnostics.HasError() {
cloudfoundry/provider/types_app.go:31
- The new struct field name
Lifecycleis ambiguous given the schema attribute islifecycle_typeand CF apps have a broader lifecycle concept (type+data). Renaming this field toLifecycleTypewould better match the attribute name and avoid confusion when adding lifecycle data support later.
Lifecycle types.String `tfsdk:"lifecycle_type"`
cloudfoundry/provider/types_app.go:204
- docker_image is Optional (and can be Unknown during planning). mapAppTypeToValues currently treats Unknown as "set" (because it only checks IsNull), which can push a manifest with an empty docker image string. This becomes more likely now that lifecycle_type can be Computed/Unknown; it should ignore docker_image when it's Unknown.
This issue also appears on line 394 of the same file.
appmanifest.Docker = &appManifestDocker
}
if !appType.Lifecycle.IsNull() && !appType.Lifecycle.IsUnknown() {
appmanifest.Lifecycle = cfv3operation.AppLifecycle(appType.Lifecycle.ValueString())
}
This duplication already existed in the codebase before this PR ( |
|
@SMendaci Could we restrict lifecycle_type to a defined set of allowed values? The CF v3 API only supports three lifecycle types — buildpack, docker, and cnb — and the current implementation accepts any arbitrary string. Adding an enum constraint would catch misconfigured values at plan time rather than surfacing a CF API error at apply time. Additionally, could you add VCR-recorded tests covering the new lifecycle_type attribute? You can spin up a local CF environment using kind-deployment to record the fixtures. |
| EnableSSH types.Bool `tfsdk:"enable_ssh"` | ||
| Stack types.String `tfsdk:"stack"` | ||
| Buildpacks types.List `tfsdk:"buildpacks"` | ||
| Lifecycle types.String `tfsdk:"lifecycle_type"` |
There was a problem hiding this comment.
can you rename it to LifecycleType? This makes the attribute name more explicit about what it represents. Same for DatasourceAppType too.
| reqPlanType is required here to identify whether attributes like "health-check-interval", "readiness-health-check-interval" | ||
| are present as part of app spec or not, since cf api controller converts them to be part of process spec internally | ||
| */ | ||
| func mapLifecycleToStringType(appManifest *cfv3operation.AppManifest, app *cfv3resource.App) types.String { |
There was a problem hiding this comment.
Could we move mapLifecycleToStringType above these comments? These comments describe the behavior of mapAppValuesToType, so keeping them immediately above that function would make the context clearer.
|
Thanks @ANUGRAHG
|
|
Hi @SMendaci, |
Hi @ANUGRAHG |
|
Hello @SMendaci , |
Hello @ANUGRAHG , |
|
Hi @SMendaci There’s no need to record all the tests. You can only record the newly added test cases. |
|
|
Hi @ANUGRAHG , |
Purpose
Cloud Foundry already supports the
lifecycleattribute in application manifests. This change exposes that support through the provider aslifecycle_type, including thebuildpack,cnb, anddockervariants.lifecycle_typeoncloudfoundry_appso lifecycle configuration is read from and written to the app manifest mapping.lifecycle_typeto the allowed valuesbuildpack,docker, andcnbto catch invalid values at plan time.lifecycle_typeincloudfoundry_appandcloudfoundry_appsdata sources for read parity with the resource.lifecycle_typebehavior.Does this introduce a breaking change?
Pull Request Type
What kind of change does this Pull Request introduce?
How to Test
lifecycle_type = "docker"withdocker_imageset should pass validation.lifecycle_type = "docker"withoutdocker_imageshould fail validation.docker_imagewith a non-dockerlifecycle_typeshould fail validation.lifecycle_type = "cnb"with a buildpack app should be accepted.What to Check
Verify that the following are valid:
lifecycle_typeis persisted/read correctly oncloudfoundry_applifecycle_typeis exposed by both app data sourcesOther Information
cnblifecycle variant is now explicitly supported and covered by VCR tests.Checklist for reviewer
The following organizational tasks must be completed before merging this PR: