Automate local discovery and asset generation for Cloud Foundry app#140
Automate local discovery and asset generation for Cloud Foundry app#140nachandr wants to merge 1 commit intokonveyor:mainfrom
Conversation
📝 WalkthroughWalkthroughIntroduces Cloud Foundry manifest and corresponding Helm chart infrastructure for the hello-spring-cloud application, including deployment templates, configuration values, helper functions, and end-to-end testing for offline discovery and asset generation workflows. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
nachandr
force-pushed
the
cf_local_discovery
branch
from
35bb2ac to
8cfaf20
Compare
Signed-off-by: Nandini Chandra <nachandr@redhat.com>
nachandr
force-pushed
the
cf_local_discovery
branch
from
8cfaf20 to
d8633bd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@helm-charts/hello-spring-cloud/templates/_helpers.tpl`:
- Around line 57-67: Remove the dead duplicate assignment of $args made with
"$args := rest $words" in the else branch; keep the single later assignment that
drives the args block. Specifically, delete the first "$args := rest $words"
after the command section so the template only defines $args once (the "$args :=
rest $words" used before the "{{- if $args -}} ... {{- end -}}" block), ensuring
the command uses "{{ first $words | quote }}" and the args loop uses the single
$args variable.
In `@helm-charts/hello-spring-cloud/templates/deployment.yaml`:
- Around line 150-157: The livenessProbe always calls the template
hello-spring-cloud.executableName with $sidecar.command even when
$sidecar.command is nil, producing an empty grep; wrap the livenessProbe (or at
minimum the exec.command line that calls hello-spring-cloud.executableName) in
the same conditional used elsewhere ({{- if $sidecar.command }} ... {{- end }}),
ensuring you only render the exec probe when $sidecar.command is present so grep
never receives an empty string.
- Around line 20-23: Remove the invalid Kubernetes field block that emits
"recreateParams" when $.Values.deploymentStrategy.type == "Recreate" (the
template using recreateParams and $.Values.deploymentStrategy.timeout) because
recreateParams is an OpenShift DeploymentConfig concept and not valid for
apps/v1 Deployment; either delete that conditional block entirely, or replace it
by mapping the intent to valid Deployment fields such as spec.minReadySeconds or
spec.progressDeadlineSeconds (use $.Values.deploymentStrategy.timeout as
appropriate) and update the template to render those valid fields instead of
recreateParams.
In `@tests/platform/cloudfoundry/test_local_discovery.py`:
- Around line 28-32: The code re-fetches the environment variable instead of
reusing the earlier local variable; change the construction of manifest_file and
helm_chart_dir to use the existing project_path variable (referencing
manifest_file, helm_chart_dir and project_path) so both os.path.join calls use
project_path rather than calling os.getenv(constants.PROJECT_PATH) again.
- Around line 81-84: Wrap the test body in a try/finally (or implement a pytest
teardown fixture) so cleanup always runs; specifically ensure that
discovery_output_dir and asset_dir are removed in the finally block (or fixture
teardown) regardless of failures. Locate the test in test_local_discovery.py and
move the existing shutil.rmtree calls for discovery_output_dir and asset_dir
into the finally block (or a fixture finalizer) so stale directories are removed
even if assertions or subprocess.run(check=True) raise.
🧹 Nitpick comments (5)
helm-charts/hello-spring-cloud/generated-manifests.yaml (2)
1-72: Consider not committing generated manifests.
generated-manifests.yamlis ahelm templateoutput artifact that can always be regenerated. Committing it risks drift if someone updates the templates/values but forgets to regenerate this file. If it's used as a test fixture for assertions, that's reasonable — but consider adding a comment at the top explaining its purpose and how to regenerate it.
55-64: No resource requests or limits defined on the container.Without
resources.requestsandresources.limits, the pod has no QoS guarantees and could be evicted first under memory pressure, or consume unbounded resources. Consider adding defaults invalues.yamland wiring them into the deployment template.helm-charts/hello-spring-cloud/templates/deployment.yaml (1)
35-70: NosecurityContextconfigured at pod or container level.Trivy flagged that the container runs without a security context, meaning it could run as root with a writable root filesystem. For a reference/test chart this may be acceptable, but consider adding at least a minimal security context to align with best practices.
Example addition
spec: terminationGracePeriodSeconds: {{ $.Values.terminationGracePeriod | default 60 }} serviceAccountName: {{ include "hello-spring-cloud.serviceAccountName" $ }} + securityContext: + runAsNonRoot: true containers: - name: {{ $.Values.manifest.name }}-{{ $process.type }} image: "{{ $.Values.image.registry }}/{{ $.Values.manifest.name }}-{{ $process.type }}:{{ $.Values.image.tag }}" imagePullPolicy: {{ $.Values.image.pullPolicy }} + securityContext: + readOnlyRootFilesystem: true + allowPrivilegeEscalation: falsetests/platform/cloudfoundry/test_local_discovery.py (1)
22-25: Preferpytest.skip()over raisingExceptionfor missing environment preconditions.When
PROJECT_PATHorCLOUDFOUNDRY_FILES_PATHis not set, the test can't run but that's a configuration issue, not a test failure. Usingpytest.skip()gives a clearer signal in CI (skipped vs errored).Proposed fix
if not project_path: - raise Exception("PROJECT_PATH environment variable is not set") + pytest.skip("PROJECT_PATH environment variable is not set") if not cf_files_path: - raise Exception("CLOUDFOUNDRY_FILES_PATH environment variable is not set") + pytest.skip("CLOUDFOUNDRY_FILES_PATH environment variable is not set")helm-charts/hello-spring-cloud/templates/_helpers.tpl (1)
19-28:executableNameregex matches any command containing a dot, not just path-prefixed commands.The regex
\..*will match any$lastPartthat contains a literal dot (e.g.,app.jar,server.sh). For such cases,regexFind '[^/]+$'just returns the same string since there's no/to strip. This works correctly for the current use cases (./tr-server→tr-server), but the docstring claim of "handling file extensions" is misleading — extensions are preserved, only path prefixes are stripped.Not a bug, just a documentation accuracy note.
| {{- else -}} | ||
| {{- $args := rest $words -}} | ||
| command: | ||
| - {{ first $words | quote }} | ||
| {{ $args := rest $words -}} | ||
| {{- if $args -}} | ||
| args: | ||
| {{- range $args }} | ||
| - {{ . | quote }} | ||
| {{- end }} | ||
| {{- end -}} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Duplicate $args assignment — line 58 is dead code.
$args is assigned on line 58 ($args := rest $words) and then immediately reassigned on line 61 with the exact same expression. The first assignment is unused.
Proposed fix
{{- else -}}
-{{- $args := rest $words -}}
command:
- {{ first $words | quote }}
{{ $args := rest $words -}}🤖 Prompt for AI Agents
In `@helm-charts/hello-spring-cloud/templates/_helpers.tpl` around lines 57 - 67,
Remove the dead duplicate assignment of $args made with "$args := rest $words"
in the else branch; keep the single later assignment that drives the args block.
Specifically, delete the first "$args := rest $words" after the command section
so the template only defines $args once (the "$args := rest $words" used before
the "{{- if $args -}} ... {{- end -}}" block), ensuring the command uses "{{
first $words | quote }}" and the args loop uses the single $args variable.
| {{- if eq $.Values.deploymentStrategy.type "Recreate" }} | ||
| recreateParams: | ||
| timeoutSeconds: {{ $.Values.deploymentStrategy.timeout }} | ||
| {{- end }} |
There was a problem hiding this comment.
recreateParams is not a valid field for Kubernetes Deployment.
The Kubernetes Recreate strategy type has no configurable sub-parameters. recreateParams is an OpenShift DeploymentConfig concept, not a standard apps/v1 Deployment field. Kubernetes will silently ignore this block, making it dead configuration that could mislead future maintainers.
Either remove it, or if you need timeout semantics, consider using spec.minReadySeconds or spec.progressDeadlineSeconds instead.
Proposed fix
{{- end }}
- {{- if eq $.Values.deploymentStrategy.type "Recreate" }}
- recreateParams:
- timeoutSeconds: {{ $.Values.deploymentStrategy.timeout }}
- {{- end }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {{- if eq $.Values.deploymentStrategy.type "Recreate" }} | |
| recreateParams: | |
| timeoutSeconds: {{ $.Values.deploymentStrategy.timeout }} | |
| {{- end }} |
🤖 Prompt for AI Agents
In `@helm-charts/hello-spring-cloud/templates/deployment.yaml` around lines 20 -
23, Remove the invalid Kubernetes field block that emits "recreateParams" when
$.Values.deploymentStrategy.type == "Recreate" (the template using
recreateParams and $.Values.deploymentStrategy.timeout) because recreateParams
is an OpenShift DeploymentConfig concept and not valid for apps/v1 Deployment;
either delete that conditional block entirely, or replace it by mapping the
intent to valid Deployment fields such as spec.minReadySeconds or
spec.progressDeadlineSeconds (use $.Values.deploymentStrategy.timeout as
appropriate) and update the template to render those valid fields instead of
recreateParams.
| {{- /* Sidecars have an implicit health check of type "process"*/}} | ||
| livenessProbe: | ||
| exec: | ||
| command: | ||
| - sh | ||
| - -c | ||
| - "ps -ef | grep '{{ include "hello-spring-cloud.executableName" $sidecar.command }}' | grep -v grep" | ||
| initialDelaySeconds: {{ $process.healthCheck.timeout }} |
There was a problem hiding this comment.
Sidecar livenessProbe unconditionally references $sidecar.command, but it may be nil.
Line 127 correctly gates command parsing with {{- if $sidecar.command }}, but line 156 always calls hello-spring-cloud.executableName with $sidecar.command regardless. If a sidecar has no command defined, this will produce a broken grep '' probe.
Proposed fix — gate the sidecar livenessProbe on command presence
{{- /* Sidecars have an implicit health check of type "process"*/}}
livenessProbe:
+ {{- if $sidecar.command }}
exec:
command:
- sh
- -c
- "ps -ef | grep '{{ include "hello-spring-cloud.executableName" $sidecar.command }}' | grep -v grep"
initialDelaySeconds: {{ $process.healthCheck.timeout }}
+ {{- else }}
+ tcpSocket:
+ port: {{ $.Values.port }}
+ {{- end }}🤖 Prompt for AI Agents
In `@helm-charts/hello-spring-cloud/templates/deployment.yaml` around lines 150 -
157, The livenessProbe always calls the template
hello-spring-cloud.executableName with $sidecar.command even when
$sidecar.command is nil, producing an empty grep; wrap the livenessProbe (or at
minimum the exec.command line that calls hello-spring-cloud.executableName) in
the same conditional used elsewhere ({{- if $sidecar.command }} ... {{- end }}),
ensuring you only render the exec probe when $sidecar.command is present so grep
never receives an empty string.
| manifest_file = os.path.join(os.getenv(constants.PROJECT_PATH), 'data', 'yaml', 'platform', 'cloudfoundry', 'hello-spring-cloud-manifest.yml') | ||
| if not os.path.exists(manifest_file): | ||
| raise Exception(f"Manifest file not found: {manifest_file}") | ||
|
|
||
| helm_chart_dir = os.path.join(os.getenv(constants.PROJECT_PATH), 'helm-charts') |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Reuse the project_path local variable instead of calling os.getenv again.
Lines 28 and 32 call os.getenv(constants.PROJECT_PATH) even though line 19 already captured the result in project_path.
Proposed fix
- manifest_file = os.path.join(os.getenv(constants.PROJECT_PATH), 'data', 'yaml', 'platform', 'cloudfoundry', 'hello-spring-cloud-manifest.yml')
+ manifest_file = os.path.join(project_path, 'data', 'yaml', 'platform', 'cloudfoundry', 'hello-spring-cloud-manifest.yml')
if not os.path.exists(manifest_file):
raise Exception(f"Manifest file not found: {manifest_file}")
- helm_chart_dir = os.path.join(os.getenv(constants.PROJECT_PATH), 'helm-charts')
+ helm_chart_dir = os.path.join(project_path, 'helm-charts')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| manifest_file = os.path.join(os.getenv(constants.PROJECT_PATH), 'data', 'yaml', 'platform', 'cloudfoundry', 'hello-spring-cloud-manifest.yml') | |
| if not os.path.exists(manifest_file): | |
| raise Exception(f"Manifest file not found: {manifest_file}") | |
| helm_chart_dir = os.path.join(os.getenv(constants.PROJECT_PATH), 'helm-charts') | |
| manifest_file = os.path.join(project_path, 'data', 'yaml', 'platform', 'cloudfoundry', 'hello-spring-cloud-manifest.yml') | |
| if not os.path.exists(manifest_file): | |
| raise Exception(f"Manifest file not found: {manifest_file}") | |
| helm_chart_dir = os.path.join(project_path, 'helm-charts') |
🧰 Tools
🪛 Ruff (0.15.0)
[warning] 30-30: Create your own exception
(TRY002)
[warning] 30-30: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In `@tests/platform/cloudfoundry/test_local_discovery.py` around lines 28 - 32,
The code re-fetches the environment variable instead of reusing the earlier
local variable; change the construction of manifest_file and helm_chart_dir to
use the existing project_path variable (referencing manifest_file,
helm_chart_dir and project_path) so both os.path.join calls use project_path
rather than calling os.getenv(constants.PROJECT_PATH) again.
| if os.path.exists(discovery_output_dir): | ||
| shutil.rmtree(discovery_output_dir) | ||
| if os.path.exists(asset_dir): | ||
| shutil.rmtree(asset_dir) |
There was a problem hiding this comment.
Cleanup won't run if the test fails mid-execution.
If an assertion or subprocess.run(check=True) raises, lines 81–84 are never reached and stale directories remain. Wrap the test body in try/finally or use a pytest fixture for teardown.
Proposed fix using try/finally
+ try:
discovery_command = build_platform_local_discovery_command(
manifest_file=manifest_file,
output_dir=discovery_output_dir
)
# ... rest of test body ...
print(f"Successfully generated {len(asset_files)} asset file(s)")
+ finally:
+ if os.path.exists(discovery_output_dir):
+ shutil.rmtree(discovery_output_dir)
+ if os.path.exists(asset_dir):
+ shutil.rmtree(asset_dir)
-
- if os.path.exists(discovery_output_dir):
- shutil.rmtree(discovery_output_dir)
- if os.path.exists(asset_dir):
- shutil.rmtree(asset_dir)🤖 Prompt for AI Agents
In `@tests/platform/cloudfoundry/test_local_discovery.py` around lines 81 - 84,
Wrap the test body in a try/finally (or implement a pytest teardown fixture) so
cleanup always runs; specifically ensure that discovery_output_dir and asset_dir
are removed in the finally block (or fixture teardown) regardless of failures.
Locate the test in test_local_discovery.py and move the existing shutil.rmtree
calls for discovery_output_dir and asset_dir into the finally block (or a
fixture finalizer) so stale directories are removed even if assertions or
subprocess.run(check=True) raise.
|
1 participant
Summary by CodeRabbit
Release Notes
New Features
Tests