[Misc] Conditional logging for helm chart generation message#89
[Misc] Conditional logging for helm chart generation message#89anirudhprasad-sap merged 1 commit intomainfrom
Conversation
![hyperspace-insights[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
There was a problem hiding this comment.
Now I have a thorough understanding of the full context. Let me analyze the logic:
The new condition !isConfigurable && !withTemplates:
-
isConfigurable(--with-configurable-templates): Creates a chart with embedded templates (chart/templates/). Since templates are already inlined,cds buildis not needed to get the helm chart. Suppressing the message is correct. -
withTemplates(--with-templates):createSimpleChartis called withwithTemplates=true, which also copies templates intochart/templates/. Same as above —cds buildis not needed. Suppressing is correct. -
Neither flag (simple chart without templates):
cds buildis needed to generate the chart from values.yaml. Message should show. ✅ -
--with-mtacase: Note that--with-mtaand--with-configurable-templatesare not mutually exclusive pervalidateOptions. When--with-mtais used without--with-configurable-templatesand without--with-templates, the "run cds build" message still shows. This is technically correct because a simple chart without templates does still requirecds build. However, the MTA case already gives the user a different message ("Deriving values.yaml from mta.yaml cannot be done one to one..."), so showing the "run cds build" message afterward is also reasonable.
The logic looks correct. Let me also double-check: can isConfigurable be truthy when withTemplates is also truthy? No — validateOptions throws an error when both are specified together. So the || in the condition covers all valid cases.
One issue I do find: The condition !isConfigurable && !withTemplates uses negated logic which can be hard to parse. But more critically, it also means the message is suppressed when --with-mta is used with a template-related flag — but that's actually fine because --with-mta + --with-configurable-templates would produce a configurable chart (with inline templates, no cds build needed). This is correct.
The code looks logically sound. However, I notice one actual correctness issue: --with-mta with a plain (non-template, non-configurable) chart should arguably also not show the "run cds build" message because handleMtaIntegration already handles the output guidance and the MTA path populates values.yaml automatically. Let me reconsider — actually, the cds build step is still needed to generate the rendered Helm chart even with MTA. The message "Once values.yaml is updated, run 'cds build'..." still applies after MTA population. So this is correct.
The change looks correct. Let me verify if there's any issue with the --with-mta + --with-configurable-templates combination — validateOptions does NOT block this combination, so it's possible. In that case: isConfigurable=true, so the message is suppressed, which is right. ✅
No significant issues found with the logic of this change. The PR is simple and correct.
The change is straightforward and logically sound — the "run cds build" message is correctly suppressed only for --with-configurable-templates and --with-templates cases where the helm chart templates are already embedded in the chart/ folder and a separate cds build step is not needed. No issues to flag.
PR Bot Information
Version: 1.19.9 | 📖 Documentation | 🚨 Create Incident | 💬 Feedback
- Correlation ID:
bd1d6ca0-2c3f-11f1-9133-bd2d10117470 - File Content Strategy: Full file content
- LLM:
anthropic--claude-4.6-sonnet - Event Trigger:
pull_request.opened
|
2 participants
Conditional Logging for Helm Chart Generation Message
Bug Fix
🐛 Fixed an unnecessary log message being displayed unconditionally during helm chart generation. The message instructing users to run
cds buildis now only shown when applicable — specifically when the chart is neither configurable nor generated with templates.Changes
lib/add.js: Wrapped the"Once values.yaml is updated, run 'cds build'..."console log in a conditional check so it only prints whenisConfigurableisfalseandwithTemplatesisfalse. This avoids showing a misleading or irrelevant message in configurable chart or template-based scenarios.📬 Subscribe to the Hyperspace PR Bot DL to get the latest announcements and pilot features!
PR Bot Information
Version:
1.19.9| 📖 Documentation | 🚨 Create Incident | 💬 Feedbackanthropic--claude-4.6-sonnetpull_request.openedbd1d6ca0-2c3f-11f1-9133-bd2d10117470