Asset generation for Cloud Foundry Applications #215

jordigilh · 2025-01-23T16:31:39Z

@savitharaghunathan @rromannissen @jortel @dymurray @eemcmullan @JonahSussman please review.

@pkliczewski FYI

Signed-off-by: Jordi Gil <[email protected]>

enhancements/assets-generation/cloud-foundry-application-discovery/README.md

Signed-off-by: Jordi Gil <[email protected]>

* BuildPacks * Space.Version * Service.BindingName Signed-off-by: Jordi Gil <[email protected]>

rromannissen

Really looks good to me, very thorough documentation of the proposed canonical configuration manifest spec. Only some minor changes to do and we should be good to go IMHO.

enhancements/assets-generation/cloud-foundry-application-discovery/README.md

jordigilh

Updated with suggestions

enhancements/assets-generation/cloud-foundry-application-discovery/README.md

Signed-off-by: Jordi Gil <[email protected]>

…luralized names. Signed-off-by: Jordi Gil <[email protected]>

Signed-off-by: Jordi Gil <[email protected]>

jortel · 2025-01-27T15:02:42Z

enhancements/assets-generation/cloud-foundry-application-discovery/README.md

+
+### Proposal Specification
+
+#### Space specification


Space seems CF oriented and wondering if Namespace would be more generic and better aligned with konveyor vernacular?

We had a similar thought on several situations. We kept the source name for:

Process -> Container

Sidecar -> Container
But we decided to change for:

Instances -> Replicas`. At the time we thought to align with Kubernetes domain name, but it was a long debate and still ongoing, hopefully this comment can help us decide 😄

Timeout -> StartupTimeout (This one we renamed because timeout didn't convey it's intent, compared to the other timeout field instances found in the application health and readiness checks)

So, in a nutshell, what is the normal expectation in the discovery? use a nomenclature that is aligned with the source or with konveyor so that all discovery outputs share a common naming convention?

I'd say we should use a nomenclature aligned with the source platform terminology. For me, it doesn't make sense to try to change the source names, since that would introduce some bias from our side on how we see the conversion happening, when that should be totally up to the templates author.

It's my understanding that the proposal is for the canonical form which is intended to be source agnostic. An abstraction. So "use a nomenclature aligned with the source platform terminology. " does not make sense to me. If anything, it seems better to be biased toward kubernetes nomenclature which is well-known to the konveyor community. Also, migration to kube is our core use case.
Am I missing something?

After more discussion it sounds like "canonical configuration" has confused most of us. We now agree this is the specific platform configuration for PCF; and the naming should change. It is not source agnostic; and will be well documented for PCF

Discovery manifest should do.

if @jortel and @dymurray are fine, I'll update the document to refer to the canonical form as discovery manifest.

Good by me!

Replaced to discovery manifest

…er` structure at the `Application` level Signed-off-by: Jordi Gil <[email protected]>

jordigilh · 2025-01-28T13:09:21Z

@rromannissen is it ok if we don't interpret the default values in the CF manifests when not provided in the source manifest? For instance, the number of instances defaults to 1 when no value is provided in the CF manifest.

Signed-off-by: Jordi Gil <[email protected]>

…o struct name Signed-off-by: Jordi Gil <[email protected]>

dymurray · 2025-01-29T19:41:40Z

I'd suggest that we rename this enhancement to specifically cover asset generation and the definition of the canonical configuration. The example of using cloud foundry as the source should be a specific example called out in the enhancement, but not the title of the enhancement

…ce to Timeout Signed-off-by: Jordi Gil <[email protected]>

rromannissen · 2025-01-30T10:59:39Z

@rromannissen is it ok if we don't interpret the default values in the CF manifests when not provided in the source manifest? For instance, the number of instances defaults to 1 when no value is provided in the CF manifest.

I think we need to take that into account, because default values might be different in the target platform. In other words, if default for instances is 1, we need to explicitly populate the canonical manifest with that number, even if it's missing in the source deployment manifest.

jortel · 2025-01-30T14:25:32Z

enhancements/assets-generation/cloud-foundry-application-discovery/README.md

+  // ProcessTypes captures the different process types defined for the sidecar.
+  // Compared to a Process, which has only one type, sidecar processes can
+  // accumulate more than one type.
+  ProcessTypes []ProcessType `yaml:"processType" validate:"required,oneof=worker web"`


Enum: (worker|web) feel CF specific.

I'm not sure I'm following you: Is your comment related to the fact that we're following the names for CF process types?

eemcmullan

The enhancement looks good and detailed so far. Thank you for putting it together. I am wondering about @dymurray and @jortel's comments on this enhancement: aiming to be more standardized towards any platform, while using CF as an example scenario. In which case, some of these configs could be implemented as general platform types, with optional fields for platform-specific manifest fields. I'm interested to know of any thoughts on this.

jordigilh · 2025-01-30T15:40:10Z

The enhancement looks good and detailed so far. Thank you for putting it together. I am wondering about @dymurray and @jortel's comments on this enhancement: aiming to be more standardized towards any platform, while using CF as an example scenario. In which case, some of these configs could be implemented as general platform types, with optional fields for platform-specific manifest fields. I'm interested to know of any thoughts on this.

My impression was that we would have a canonical form for each source discovery platform. The benefit of this is that each source platform is contained within its own canonical form, and since the discovery captures the source fields verbatim, it makes it an easy task to map each field back to the source. For instance, mandatory fields in CF might not apply to other platforms, which makes the validation of the canonical form difficult because all fields will have to be optional by default in the schema, and the validation is then implemented in the code.

New source platforms can be discovered to their own canonical form. The process of interpreting the data then falls into the transformation. With Helm, this is done in the templates. I also find it easier to maintain a specific canonical form per platform in terms of code, testing and documentation.

The downside from using a generic canonical form for all platforms is that it becomes an effort in the discovery to map the values, because each source field needs to be now interpreted and mapped to the canonical form, if possible. And if it's not clearly possible, it can spawn a new field in the generic canonical form to encapsulate this meaning. Additionally, the canonical form is no longer clearly defined, since it is extendable and each new platform added can extend the canonical form by adding new fields that are not relevant to other platforms.

rromannissen · 2025-01-31T19:38:40Z

I think @jordigilh nailed it with its explanation. If we were to find a generic canonical form to store data, we would be implicitly forcing a certain transformation path, and that should be entirely up to the templates author.

dymurray · 2025-02-03T14:59:13Z

@jordigilh I much better understand the content of this enhancement now. So IIUC, the high level summary is that there will exist a canonical configuration format which has a subsection cloudFoundry (for example). That subsection dictionary is what is defined in this enhancement.

I was slightly confused at first thinking this was the definition of the root level canonical configuration being heavily guided by CF data. I now understand that you intend for this to be a platform specific section in the config. We will define the canonical configuration spec in a separate enhancement.

dymurray

Just some mild changes requested; but overall this looks really good. I followed the example PoC and it made a lot sense. After these comments are addressed I will feel good about it

dymurray · 2025-02-03T21:07:18Z

enhancements/assets-generation/cloud-foundry-application-discovery/README.md

+superseded-by:
+---
+
+# Cloud Foundry Application Discovery to canonical form


I know this is pedantic; but the more I understand about this enhancement I don't like the term "canonical configuration". In general; the term I like for this is "manifest". We are talking about converting a CF application runtime configuration into a yaml file that can be used for app deployment on a new platform. "Canonical configuration" doesn't really define this

Let me know what name the team decides to use and we'll make the changes in the document.

Discovery manifest works for me!

@dymurray since you added the comment, is it OK to rename canonical form to discovery manifest in the document?

I like discovery manifest, please!

enhancements/assets-generation/cloud-foundry-application-discovery/README.md

dymurray · 2025-02-03T21:53:41Z

enhancements/assets-generation/cloud-foundry-application-discovery/README.md

+
+### Proposal Specification
+
+#### Space specification


After more discussion it sounds like "canonical configuration" has confused most of us. We now agree this is the specific platform configuration for PCF; and the naming should change. It is not source agnostic; and will be well documented for PCF

enhancements/assets-generation/cloud-foundry-application-discovery/README.md

Signed-off-by: Jordi Gil <[email protected]>

…ifest generated from the discovery action is to produce an output that is consumed by the 'generate' operation to render the final assets Signed-off-by: Jordi Gil <[email protected]>

rromannissen

Looks great to me after the changes, I think this is ready to merge!

rromannissen · 2025-02-07T11:07:56Z

enhancements/assets-generation/cloud-foundry-application-discovery/README.md

+
+### Proposal Specification
+
+#### Space specification


Discovery manifest should do.

dymurray

Approved, the name change is good with me too if we decide that's the path forward. Everything else looks good.

Signed-off-by: Jordi Gil <[email protected]>

jordigilh · 2025-02-10T21:24:36Z

@dymurray @rromannissen canonical form renamed to discovery manifest

jordigilh force-pushed the assert_generation_cf branch from 22aa13c to 89f6120 Compare January 23, 2025 16:32

Assert generation for Cloud Foundry Applications

a0b5b55

Signed-off-by: Jordi Gil <[email protected]>

jordigilh force-pushed the assert_generation_cf branch from 89f6120 to a0b5b55 Compare January 23, 2025 16:32

savitharaghunathan reviewed Jan 23, 2025

View reviewed changes

Removed text coming from enhancement and changed MTA to Konveyor

72c882c

Signed-off-by: Jordi Gil <[email protected]>

jordigilh force-pushed the assert_generation_cf branch from b017e9b to 72c882c Compare January 23, 2025 19:15

jordigilh changed the title ~~Assert generation for Cloud Foundry Applications~~ Asset generation for Cloud Foundry Applications Jan 24, 2025

Added captured fields:

c0a1452

* BuildPacks * Space.Version * Service.BindingName Signed-off-by: Jordi Gil <[email protected]>

rromannissen requested changes Jan 24, 2025

View reviewed changes

enhancements/assets-generation/cloud-foundry-application-discovery/README.md Show resolved Hide resolved

enhancements/assets-generation/cloud-foundry-application-discovery/README.md Show resolved Hide resolved

jordigilh commented Jan 24, 2025

View reviewed changes

enhancements/assets-generation/cloud-foundry-application-discovery/README.md Show resolved Hide resolved

enhancements/assets-generation/cloud-foundry-application-discovery/README.md Show resolved Hide resolved

jordigilh force-pushed the assert_generation_cf branch from 1bf4a47 to 82a6211 Compare January 24, 2025 21:45

jordigilh added 4 commits January 24, 2025 16:46

Removed 'Mapped' column

917e855

Signed-off-by: Jordi Gil <[email protected]>

Use slices of structures to represent multiple instances instead of p…

aa5e63a

…luralized names. Signed-off-by: Jordi Gil <[email protected]>

Added golang validate annotations

e64d2a3

Signed-off-by: Jordi Gil <[email protected]>

Added Docker specification

d1f0ac0

Signed-off-by: Jordi Gil <[email protected]>

jordigilh force-pushed the assert_generation_cf branch from 82a6211 to d1f0ac0 Compare January 24, 2025 21:46

jortel reviewed Jan 27, 2025

View reviewed changes

Removed Image field in Process now that it's contained in the `Dock…

46000d5

…er` structure at the `Application` level Signed-off-by: Jordi Gil <[email protected]>

jordigilh added 2 commits January 28, 2025 08:12

Fix annotation typo for Command

e93c950

Signed-off-by: Jordi Gil <[email protected]>

Added Docker field to the Application spec

72f7457

Signed-off-by: Jordi Gil <[email protected]>

jordigilh force-pushed the assert_generation_cf branch from 7afdad7 to 72f7457 Compare January 28, 2025 13:13

jordigilh added 2 commits January 28, 2025 10:54

Replace StartupTimeout to Timeout to match the source field name

b5c9bb7

Signed-off-by: Jordi Gil <[email protected]>

Make 'type' mandatory for Process and align 'route' json field name t…

9c8c50f

…o struct name Signed-off-by: Jordi Gil <[email protected]>

Added Instances to the Application and changed StartupTimeout referen…

37712a2

…ce to Timeout Signed-off-by: Jordi Gil <[email protected]>

jortel reviewed Jan 30, 2025

View reviewed changes

eemcmullan reviewed Jan 30, 2025

View reviewed changes

dymurray requested changes Feb 3, 2025

View reviewed changes

jordigilh force-pushed the assert_generation_cf branch from 98e5a53 to 0122233 Compare February 6, 2025 20:38

jordigilh added 3 commits February 6, 2025 15:38

Change json to yaml in go struct annotation

a48ad51

Signed-off-by: Jordi Gil <[email protected]>

Add discovery manifest example and address other comments

6de6a86

Signed-off-by: Jordi Gil <[email protected]>

Add blurb in Summary section to mention the goal of the discovery man…

d6386d7

…ifest generated from the discovery action is to produce an output that is consumed by the 'generate' operation to render the final assets Signed-off-by: Jordi Gil <[email protected]>

jordigilh force-pushed the assert_generation_cf branch from 0122233 to d6386d7 Compare February 6, 2025 20:39

rromannissen reviewed Feb 7, 2025

View reviewed changes

rromannissen mentioned this pull request Feb 7, 2025

Create Asset Generation CLI Enhancement #214

Open

dymurray approved these changes Feb 10, 2025

View reviewed changes

Rename references from canonical form to discovery manifest

dfdedc4

Signed-off-by: Jordi Gil <[email protected]>

jordigilh force-pushed the assert_generation_cf branch from 6cab527 to dfdedc4 Compare February 10, 2025 21:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Asset generation for Cloud Foundry Applications #215

Asset generation for Cloud Foundry Applications #215

jordigilh commented Jan 23, 2025 •

edited

Loading

rromannissen left a comment

jordigilh left a comment

jortel Jan 27, 2025

jordigilh Jan 27, 2025

rromannissen Jan 27, 2025

jortel Jan 30, 2025 •

edited

Loading

dymurray Feb 3, 2025

rromannissen Feb 7, 2025

jordigilh Feb 7, 2025

dymurray Feb 10, 2025

jordigilh Feb 10, 2025

jordigilh commented Jan 28, 2025

dymurray commented Jan 29, 2025

rromannissen commented Jan 30, 2025

jortel Jan 30, 2025

jordigilh Jan 30, 2025

eemcmullan left a comment

jordigilh commented Jan 30, 2025

rromannissen commented Jan 31, 2025

dymurray commented Feb 3, 2025

dymurray left a comment

dymurray Feb 3, 2025

jordigilh Feb 6, 2025

rromannissen Feb 7, 2025

jordigilh Feb 7, 2025

dymurray Feb 7, 2025

dymurray Feb 3, 2025

rromannissen left a comment

rromannissen Feb 7, 2025

dymurray left a comment

jordigilh commented Feb 10, 2025

Asset generation for Cloud Foundry Applications #215

Are you sure you want to change the base?

Asset generation for Cloud Foundry Applications #215

Conversation

jordigilh commented Jan 23, 2025 • edited Loading

rromannissen left a comment

Choose a reason for hiding this comment

jordigilh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jortel Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordigilh commented Jan 28, 2025

dymurray commented Jan 29, 2025

rromannissen commented Jan 30, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eemcmullan left a comment

Choose a reason for hiding this comment

jordigilh commented Jan 30, 2025

rromannissen commented Jan 31, 2025

dymurray commented Feb 3, 2025

dymurray left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rromannissen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dymurray left a comment

Choose a reason for hiding this comment

jordigilh commented Feb 10, 2025

jordigilh commented Jan 23, 2025 •

edited

Loading

jortel Jan 30, 2025 •

edited

Loading