MTA-6439 Documentation of Dockerfile Not Generated by generate helm#312
MTA-6439 Documentation of Dockerfile Not Generated by generate helm#312
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDocumentation updates: expanded Helm-chart inspection and discovery-manifest examples, revised generation command flow and verification (separate ConfigMap and Dockerfile checks), added CF manifest schema link, and extended live-discovery procedure with example commands and optional CF config usage. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/topics/mta-cli/proc_generating-deployment-manifest.adoc (1)
50-64:⚠️ Potential issue | 🟠 MajorCLI commands look incorrect and steps are redundant.
mta-cli cddoesn’t appear to be a valid command; this should likely be a plaincd. Also,cat Dockerfileappears in both “Verify the ConfigMap” and “Verify the Dockerfile,” which is confusing.✅ Suggested edit
-. Verify the ConfigMap: +. Verify the ConfigMap: + [subs="+quotes"] ---- -$ *mta-cli cd _<location_of_deployment_manifest>_ \* +$ *cd _<location_of_deployment_manifest>_* $ *cat configmap.yaml* -$ *cat Dockerfile* ---- . Verify the Dockerfile: + [subs="+quotes"] ---- -$ *mta-cli cd _<location_of_deployment_manifest>_ \* +$ *cd _<location_of_deployment_manifest>_* $ *cat Dockerfile* ----
🤖 Fix all issues with AI agents
In `@docs/topics/mta-cli/proc_generating-deployment-manifest.adoc`:
- Line 39: Fix the typos and AsciiDoc markup in the sentence that references the
Helm chart templates: change “diretory” to “directory” and replace the angled
markup `<_files/konveyor_>` with the plain AsciiDoc-friendly `_files/konveyor_`
so the sentence reads correctly when rendered (look for the string "templates
directory" and the fragments `diretory` and `<_files/konveyor_>` to locate the
exact text to update).
|
@gciavarrini Please could you look through this PR and suggest which manifests must be added? |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/topics/mta-cli/proc_generating-deployment-manifest.adoc (2)
111-111:⚠️ Potential issue | 🟠 MajorFix incorrect command syntax.
Same issue as in the ConfigMap verification step:
mta-cli cdis invalid. Use the shellcdcommand and remove the trailing\*.🔧 Proposed fix
-$ *mta-cli cd _<location_of_deployment_manifest>_ \* +$ *cd _<location_of_deployment_manifest>_* $ *cat Dockerfile*🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/topics/mta-cli/proc_generating-deployment-manifest.adoc` at line 111, Replace the invalid shell command string "*mta-cli cd _<location_of_deployment_manifest>_ \\*" with a proper shell invocation: use the standard cd command (cd _<location_of_deployment_manifest>_) and remove the surrounding asterisks and the trailing "\*" so the documentation shows a valid shell prompt example; update the line containing the literal shown command to the cleaned "cd _<location_of_deployment_manifest>_" form.
95-98:⚠️ Potential issue | 🟡 MinorRemove extraneous formatting characters.
The
\*on line 98 appears to be a formatting error. The code block should close cleanly after the line continuation on line 97.📝 Proposed fix
$ *mta-cli generate helm --chart-dir helm_sample \ --input _<path_to_discovery-manifest>_ \ ---output-dir _<location_of_deployment_manifest>_ \* +--output-dir _<location_of_deployment_manifest>_* ----🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/topics/mta-cli/proc_generating-deployment-manifest.adoc` around lines 95 - 98, Remove the stray formatting characters `\*` that follow the command in the code block; locate the line starting with "$ *mta-cli generate helm --chart-dir helm_sample \\" (the command invocation) and delete the trailing `\*` so the code block closes cleanly after the line continuation, leaving only the intended command and backslash.
🧹 Nitpick comments (1)
docs/topics/mta-cli/proc_generating-deployment-manifest.adoc (1)
47-47: Use consistent file naming.For clarity and consistency with the rest of the documentation, use the full filename
values.yamlinstead of justvalues.📝 Proposed fix
-You generate the discovery manifest before generating the deployment manifest. In the deployment manifest, the values in the `values.yaml` file override the values in the discovery manifest. If the `values` file is empty, {ProductShortName} considers the discovery manifest configuration as input to generate the deployment manifest. +You generate the discovery manifest before generating the deployment manifest. In the deployment manifest, the values in the `values.yaml` file override the values in the discovery manifest. If the `values.yaml` file is empty, {ProductShortName} considers the discovery manifest configuration as input to generate the deployment manifest.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/topics/mta-cli/proc_generating-deployment-manifest.adoc` at line 47, Update the sentence in the "proc_generating-deployment-manifest.adoc" text so it consistently uses the full filename `values.yaml` instead of the short form `values`; specifically change the phrase "the values file" (and any other occurrences of "values" referring to the file) to "the `values.yaml` file" in the paragraph discussing how the deployment manifest overrides the discovery manifest so it matches the rest of the docs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/topics/mta-cli/proc_generating-deployment-manifest.adoc`:
- Line 45: Remove the extraneous comma that follows the closing parenthesis in
the sentence describing where to save text-based templates; update the line that
mentions "text-based templates (for example, `Dockerfile`), in the
_files/konveyor_ path in the `templates` directory." to eliminate the comma
after the parenthesis so it reads smoothly (keep references to `YAML`,
`ConfigMap`, `Deployment`, `templates` directory, and `_files/konveyor` path
intact).
- Line 104: Replace the invalid snippet "$ *mta-cli cd
_<location_of_config_map>_ \*" by using the shell cd command and removing the
stray backslash-asterisk: change the text that contains "mta-cli cd" to instruct
the reader to run "cd <location_of_config_map>" (no "mta-cli" prefix and remove
the trailing "\*") so the command is a valid shell directory change.
---
Outside diff comments:
In `@docs/topics/mta-cli/proc_generating-deployment-manifest.adoc`:
- Line 111: Replace the invalid shell command string "*mta-cli cd
_<location_of_deployment_manifest>_ \\*" with a proper shell invocation: use the
standard cd command (cd _<location_of_deployment_manifest>_) and remove the
surrounding asterisks and the trailing "\*" so the documentation shows a valid
shell prompt example; update the line containing the literal shown command to
the cleaned "cd _<location_of_deployment_manifest>_" form.
- Around line 95-98: Remove the stray formatting characters `\*` that follow the
command in the code block; locate the line starting with "$ *mta-cli generate
helm --chart-dir helm_sample \\" (the command invocation) and delete the
trailing `\*` so the code block closes cleanly after the line continuation,
leaving only the intended command and backslash.
---
Nitpick comments:
In `@docs/topics/mta-cli/proc_generating-deployment-manifest.adoc`:
- Line 47: Update the sentence in the "proc_generating-deployment-manifest.adoc"
text so it consistently uses the full filename `values.yaml` instead of the
short form `values`; specifically change the phrase "the values file" (and any
other occurrences of "values" referring to the file) to "the `values.yaml` file"
in the paragraph discussing how the deployment manifest overrides the discovery
manifest so it matches the rest of the docs.
2abd907 to
6e3e760
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/topics/mta-cli/proc_generating-deployment-manifest.adoc (1)
95-98:⚠️ Potential issue | 🟡 MinorFix the trailing markup on the command.
Line 97 ends with
\*but there's no continuation line. The backslash should be removed, leaving only the closing bold marker*.📝 Proposed fix
$ *mta-cli generate helm --chart-dir helm_sample \ --input _<path_to_discovery-manifest>_ \ ---output-dir _<location_of_deployment_manifest>_ \* +--output-dir _<location_of_deployment_manifest>_* ----🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/topics/mta-cli/proc_generating-deployment-manifest.adoc` around lines 95 - 98, The trailing backslash before the closing bold marker in the command snippet "mta-cli generate helm --chart-dir helm_sample --input _<path_to_discovery-manifest>_ --output-dir _<location_of_deployment_manifest>_" is incorrect; remove the backslash so the line ends with a single asterisk (*) to close the bold markup (i.e., replace `\*` with `*`) ensuring proper AsciiDoc bold formatting.
🧹 Nitpick comments (1)
docs/topics/mta-cli/proc_generating-deployment-manifest.adoc (1)
89-89: Consider clarifying the buildpack configuration note.The phrase "in the Cloud Foundry application manifest" might be ambiguous in this context. Consider specifying "source Cloud Foundry application manifest" or rephrasing for clarity.
✏️ Alternative phrasing
-NOTE: You must configure the buildpacks in the Cloud Foundry application manifest to include the buildpacks in the discovery and deployment assets. +NOTE: Buildpacks are included in the discovery and deployment assets only if they are configured in the source Cloud Foundry application manifest.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/topics/mta-cli/proc_generating-deployment-manifest.adoc` at line 89, The NOTE's wording is ambiguous; update the sentence "NOTE: You must configure the buildpacks in the Cloud Foundry application manifest to include the buildpacks in the discovery and deployment assets." to explicitly reference the source manifest (for example: "NOTE: You must configure the buildpacks in the source Cloud Foundry application manifest to include the buildpacks in the discovery and deployment assets."), or rephrase similarly to remove ambiguity about which manifest is meant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/topics/mta-cli/proc_generating-deployment-manifest.adoc`:
- Line 111: Remove the erroneous escape sequence at the end of the bolded cd
example: find the line containing "$ *cd _<location_of_deployment_manifest>_ \*"
and delete the backslash so the trailing "\*" becomes the closing bold marker
"*" resulting in "$ *cd _<location_of_deployment_manifest>_*".
---
Outside diff comments:
In `@docs/topics/mta-cli/proc_generating-deployment-manifest.adoc`:
- Around line 95-98: The trailing backslash before the closing bold marker in
the command snippet "mta-cli generate helm --chart-dir helm_sample --input
_<path_to_discovery-manifest>_ --output-dir _<location_of_deployment_manifest>_"
is incorrect; remove the backslash so the line ends with a single asterisk (*)
to close the bold markup (i.e., replace `\*` with `*`) ensuring proper AsciiDoc
bold formatting.
---
Duplicate comments:
In `@docs/topics/mta-cli/proc_generating-deployment-manifest.adoc`:
- Line 45: There's an extraneous comma after the closing parenthesis in the
sentence that starts "You must save text-based templates (for example,
`Dockerfile`), in the _files/konveyor_ path..." — remove that comma so it reads
"...`Dockerfile`) in the _files/konveyor_ path..." (edit the line containing the
phrase "text-based templates (for example, `Dockerfile`), in the
_files/konveyor_ path" to delete the comma and ensure spacing stays correct).
- Line 104: Remove the stray escape before the closing bold marker in the line
containing "$ *cd _<location_of_config_map>_ \*": delete the backslash so the
closing asterisk remains unescaped (resulting in "$ *cd
_<location_of_config_map>_*"), ensuring the bold markup is properly closed.
---
Nitpick comments:
In `@docs/topics/mta-cli/proc_generating-deployment-manifest.adoc`:
- Line 89: The NOTE's wording is ambiguous; update the sentence "NOTE: You must
configure the buildpacks in the Cloud Foundry application manifest to include
the buildpacks in the discovery and deployment assets." to explicitly reference
the source manifest (for example: "NOTE: You must configure the buildpacks in
the source Cloud Foundry application manifest to include the buildpacks in the
discovery and deployment assets."), or rephrase similarly to remove ambiguity
about which manifest is meant.
| └── values.yaml | ||
| ---- | ||
| + | ||
| You must save the Kubernetes `YAML` templates (for example, `ConfigMap` or `Deployment`) in the `templates` directory present in the root directory of the `Helm` chart. You must save text-based templates (for example, `Dockerfile`) in the _files/konveyor_ path in the `templates` directory. |
There was a problem hiding this comment.
I would says something like "Save non-Kubernetes templates (such as Dockerfile or configuration files) in the files/konveyor/ at the root of your Helm chart.".
- I feel
text-based filesit's inaccurate - Please note that
files/konveyor/must be located in the root folder, not insidetemplatefolder.
There was a problem hiding this comment.
But the /files/konveyor dir structure within the templates folder also works? I was wondering if the structure is a very strict requirement.
I verified the Dockerfile description in the Docker documentation. Could you check again, please?
There was a problem hiding this comment.
nope it's in the root. See https://github.com/konveyor/asset-generation/tree/main/pkg/providers/generators/helm/test_data/mixed_templates/files/konveyor (where mixed_templates is the root).
And, as proof (i don't think we need to mention in the doc), that path is hardcoded in the generate library https://github.com/konveyor/asset-generation/blob/e9891d2466a276a4f9d4957bbe1e0d3c8f942d39/pkg/providers/generators/helm/provider.go#L16
There was a problem hiding this comment.
I was wondering if the structure is a very strict requirement.
yes it is
I verified the Dockerfile description in the Docker documentation. Could you check again, please?
can you clarify this? I don't understand what is your doubt about the Dockerfile
There was a problem hiding this comment.
ah, the helm chart directory structure is what the JIRA asks me to document. Sorry but I need to include it because of the requirement in the ticket.
The Docker website describes Dockerfile as: "A Dockerfile is a text-based document that's used to..."
There was a problem hiding this comment.
The Docker website describes Dockerfile as: "A Dockerfile is a text-based document that's used to..."
That's absolutely true, and you're the expert on how to write documentation! 😊
I'm just sharing my technical perspective, but you definitely know what works best for the users.
There was a problem hiding this comment.
Thanks for your feedback 😄 They're really helpful!
| + | ||
| You must save the Kubernetes `YAML` templates (for example, `ConfigMap` or `Deployment`) in the `templates` directory present in the root directory of the `Helm` chart. You must save text-based templates (for example, `Dockerfile`) in the _files/konveyor_ path in the `templates` directory. | ||
| + | ||
| When you generate generate the deployment manifest after generating the discovery manifest, the values in the `values.yaml` file override the values in the discovery manifest. If the `values` file is empty, {ProductShortName} considers the discovery manifest configuration as input to generate the deployment manifest. |
There was a problem hiding this comment.
- There is a typo
When you generate generate. - I suggest rewording a bit the initial because it's a bit unclear. Maybe "When generating deployment manifests from the discovery manifest, "
- If the
valuesfile is empty or missing - Not sure about this "considers the discovery manifest configuration": is it correct to use
configuration? I think it isn't clear that the generation uses discovery manifest values directly. WDYT?
There was a problem hiding this comment.
Yup, will make the changes. Thanks!
There was a problem hiding this comment.
Hello @gciavarrini !
- Took care of the typo
- Reworded for better clarity
- Added the word missing to describe the values.yaml file scenarios
- Removed the configuration.
Could you take a quick look at the changes, please?
| + | ||
| When you generate generate the deployment manifest after generating the discovery manifest, the values in the `values.yaml` file override the values in the discovery manifest. If the `values` file is empty, {ProductShortName} considers the discovery manifest configuration as input to generate the deployment manifest. | ||
|
|
||
| . Explore the discovery manifest configuration: |
There was a problem hiding this comment.
Got it 👍 Removed configuration in this sentence.
| + | ||
| [subs="+quotes"] | ||
| ---- | ||
| manifest: |
There was a problem hiding this comment.
not sure i get the intent here.
Do you want to show an example of the generate command output?
There was a problem hiding this comment.
The JIRA asks for a template of the Dockerfile so that developers can create templates that can capture these values.
There was a problem hiding this comment.
ahhhh ok, maybe it's better to clarify that you're providing an example for that scenario.
I'll try to explain the big picture.
The generate command needs as input a discovery manifest.
Let's say that the discovery.yaml looks like
appName: my-nodejs-app
database:
enabled: true
host: postgres.example.comExample 1: no values.yaml
you have a Dockerfile template like
FROM node:18
ENV APP_NAME={{ .Values.appName }}
{{- if .Values.database.enabled }}
ENV DATABASE_HOST={{ .Values.database.host }}
{{- end }}The generate command will render it to
FROM node:18
ENV APP_NAME=my-nodejs-app
ENV DATABASE_HOST=postgres.example.com
Example 2: with values.yaml
you have a Dockerfile template like:
FROM node:18
ENV APP_NAME={{ .Values.appName }}
{{- if .Values.database.enabled }}
ENV DATABASE_HOST={{ .Values.database.host }}
{{- end }}
{{- if .Values.redis }}
ENV REDIS_URL={{ .Values.redis.url }}
{{- end }}note that .Values.redis.url isn't available in the discovery manifest.
You also have this values.yaml file:
appName: my-nodejs-app-production # Override discovery value
database:
host: prod-postgres.company.com # Override discovery value
redis: # Add new field not in discovery
url: redis://prod-redis:6379The generate command will render it to:
FROM node:18
ENV APP_NAME=my-nodejs-app-production
ENV DATABASE_HOST=prod-postgres.company.com
ENV REDIS_URL=redis://prod-redis:6379Does this help? Please let me know if there is any unclear part :)
There was a problem hiding this comment.
Thanks very much! I documented these examples in a reference file added to this PR (because doc conventions). Could you take a look, please?
There was a problem hiding this comment.
I really like the new structure! The example now it's much more clear in my opinion: well done 👏🏻
There was a problem hiding this comment.
Well, thanks to @mpershina for helping me with the structural suggestions 😁
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
| appName: my-app-production # Overrides application name in the discovery manifest | ||
| database: | ||
| host: prod-postgres.company.com # Overrides host value in the discovery manifest | ||
| redis: # Adds a new field in the values file |
There was a problem hiding this comment.
Can you please double check if "in the values file" clear/correct? Because as a reader I think the values file is the file you're creating in those lines (83-87)
gciavarrini
left a comment
There was a problem hiding this comment.
Excellent work 👏🏻
You handled the complexity of this topic perfectly, and the document is very clear. Thanks for addressing all the previous comments so thoroughly.
I have one minor note left, but overall this is a definite LGTM.
|
Hello @rromannissen, Can you please review this PR? I think I've added the requirements in MTA-6439 Dockerfile Not Generated by generate helm. Please let me know if you're ok with it. Thanks much! |
Thank you for your detailed feedback 😃 |
docs/topics/mta-cli/ref_discover-values-deployment-manifest.adoc
Outdated
Show resolved
Hide resolved
mpershina
left a comment
There was a problem hiding this comment.
Overall, LGTM, but there are some issues with rendering (see the txt file attacjed to the corresponding comment) and general minor issues that need fixes.
Signed-off-by: Prabha Kylasamiyer Sundara Rajan <pkylasam@pkylasam-thinkpadp16vgen1.bengluru.csb>
398135c to
ac587bf
Compare
|
I see the documentation keeps referring to "deployment manifest", when it should be plural instead: "deployment manifests". Hi @rromannissen , |
|
|
||
| :_mod-docs-content-type: REFERENCE | ||
|
|
||
| [id="discover-values-deployment_{context}"] |
There was a problem hiding this comment.
I'm not sure I understand what this section is trying to explain. I assume it's about the possibility to override values, but I find it very confusing. I think it would make sense to include CLI commands with their respective flags to provide context for each scenario.
There was a problem hiding this comment.
Ok, I have converted the reference topic into a procedure to include commands for the steps. Can you please let me know if this makes sense? Thanks!
… into a procedure Signed-off-by: Prabha Kylasamiyer Sundara Rajan <pkylasam@pkylasam-thinkpadp16vgen1.bengluru.csb>
Signed-off-by: Prabha Kylasamiyer Sundara Rajan <pkylasam@pkylasam-thinkpadp16vgen1.bengluru.csb>
Signed-off-by: Prabha Kylasamiyer Sundara Rajan <pkylasam@pkylasam-thinkpadp16vgen1.bengluru.csb>
| + | ||
| [source, terminal] | ||
| ---- | ||
| $ *mta-cli generate helm --chart-dir helm_sample* \ |
There was a problem hiding this comment.
How are you referencing that values.yaml file in here? Where is that file located? How come it is overriding values from the discovery manifest if it is not being passed as a parameter in the command? The cat commands are not useful, what we need to understand here is the mechanics of overriding values. By the way, I don't think this works the way you are explaining here, because as far I can remember, the only way to override values from the discovery manifest was by using the --set flag to pass discrete values. I'll let @gciavarrini keep me honest here.
There was a problem hiding this comment.
@rromannissen Hi Ramon,
Thank you for your evaluation.
Gloria will be back only by March end. So, I did my best to try a few commands to check if I can pass the values file as a parameter as you mentioned. However, please note this command is not verified by @gciavarrini. Would you mind going through the changes to ensure that the updates cover the points you shared here? Much appreciate your feedback, thanks!
…added a reference for discovery manifest Signed-off-by: Prabha Kylasamiyer Sundara Rajan <pkylasam@pkylasam-thinkpadp16vgen1.bengluru.csb>
Signed-off-by: Prabha Kylasamiyer Sundara Rajan <pkylasam@pkylasam-thinkpadp16vgen1.bengluru.csb>
Signed-off-by: Prabha Kylasamiyer Sundara Rajan <pkylasam@pkylasam-thinkpadp16vgen1.bengluru.csb>
JIRA
Version
Preview
Summary by CodeRabbit