[1.5] Cleanup CRD fields in v1alpha2 of Release Candidate related fields by jordigilh · Pull Request #450 · rhdhorchestrator/orchestrator-helm-operator

jordigilh · 2024-12-20T20:40:56Z

Changes to schema:

Normalized subscription's source field name.
Removed isReleaseCandidate field and validations.
Removed rhdh.image field used in RC.
Removed sonataflowPlatform.image.[dataIndexImage|jobServiceImage] subfields used in RC.
Removed get-cluster-version helper used to retrieve the OCP version in the cluster
Moved rhdhOperator.subscription.targetNamespace to rhdhOperator.targetNamespace to avoid confusion as seen related to the subscription. @masayag @jenniferubah @chadcrum Please let me know about this change, since this is relocating a field not related to RC.

I think that's pretty much it, any other field that lingers should be addressed in the upcoming schema revision.

masayag · 2024-12-22T09:14:41Z

bundle/manifests/rhdh.redhat.com_orchestrators.yaml

                    type: object
+                  targetNamespace:
+                    default: rhdh-operator
+                    description: The target namespace for the backstage CR in which


For the default namespace for the Backstage CR, we should pick a different one than the target namespace of the operator, perhaps the orchestrator as default (if namespace exists) or require the user to specify if the RHDH is disabled.

On a second thought, I think we should remove it. Look at the logic where it is being referenced:

orchestrator-helm-operator/helm-charts/orchestrator/templates/network-policies.yaml

Lines 18 to 19 in 6bfbf3d

{{- else if (and (.Values.rhdhOperator.enabled .Values.rhdhOperator.targetNamespace )}}

{{- $namespace = .Values.rhdhOperator.targetNamespace }} # otherwise, use targetNamespace if defined

If the operator is enabled, why can't we use the subscription.namespace value and instead we have to have a new field? Maybe I'm missing something here, because I remember we had an extensive discussion about this field before, but my memory is failing me.

Thoughts @jenniferubah ?

We use rhdhOperator.targetNamespace because that is used the user to indicate the namespace they want the RHDH instance deployed and that can be different from the RHDH operator namespace subscription.namespace and also less confusing.

Maybe we could default the value of targetNamespace to rhdh if the operator is enabled instead of rhdh-operator since we want to keep those separate.

Ah I see it now. Thanks for refreshing my memory :)

@masayag I will change the default value to orchestrator as you suggest, to avoid the situation where the user is attempting to deploy the CR in the same namespace where rhdh is deployed by default

@masayag should we also change the default location for RHDH in the setup.sh script? Currently it'srhdh-operator.

https://github.com/jordigilh/orchestrator-helm-operator/blob/d15e518d5b3f01fe7537758b2612d9f95bf63a19/hack/setup.sh#L18-L20

I have a concern about this one, since the same script is used for all releases.
So not sure what will be the impact of it.
And yet, I'd prefer for 1.4 the default to be orchestrator.

masayag · 2024-12-22T09:52:05Z

config/crd/bases/rhdh.redhat.com_orchestrators.yaml

                    default: true
                    description: Enabled determines whether the operator should be deployed by the chart. Defaults to true
                    type: boolean
+                  targetNamespace:


same as above.

gabriel-farache

So we are removing all components related to RC for OSL and RHDH? That means we won't be able to use that as canaries to validate before they are officially GA
I see 54a11fc removed all templates already and this PR is simply removing the leftovers

Or is there a way to do that still? Is it documented? Maybe there is a RC branch to just to that in which the RC files still remain?

bundle/manifests/orchestrator-operator.clusterserviceversion.yaml

masayag · 2025-01-07T08:22:24Z

@jordigilh this PR is needed for 1.4 release, can you please rebase/address comments to finalize it?

…e under orchestrator.sonataflowPlatform.jobServiceImage and orchestrator.sonataflowPlatform.dataIndexImage

jordigilh · 2025-01-07T21:30:22Z

So we are removing all components related to RC for OSL and RHDH? That means we won't be able to use that as canaries to validate before they are officially GA I see 54a11fc removed all templates already and this PR is simply removing the leftovers

Or is there a way to do that still? Is it documented? Maybe there is a RC branch to just to that in which the RC files still remain?

I think RC is main now, whatever version it is, so there's no need to support this capability anymore. In the past we didn't branch the charts when we were working with 1.1 and 1.2 (first operator release) at the same time, so we had to expose these values.

jordigilh · 2025-01-07T21:30:41Z

@jordigilh this PR is needed for 1.4 release, can you please rebase/address comments to finalize it?

Done rebasing. Addressing the comments now

…tNamespace

jordigilh · 2025-01-08T14:30:19Z

Tested with helm install --dry-run=server with success :)

jordigilh · 2025-01-08T14:43:50Z

This PR should be merged in milestone-5, changing it to draft to avoid accidental merge

masayag · 2025-01-08T15:01:48Z

This PR should be merged in milestone-5, changing it to draft to avoid accidental merge

Somehow I thought we wanted this to 1.4. Thanks for the clarification.

jordigilh · 2025-01-08T15:04:57Z

This PR should be merged in milestone-5, changing it to draft to avoid accidental merge

Somehow I thought we wanted this to 1.4. Thanks for the clarification.

I thought so, but since we're in RC for 1.4, I think it's not a good idea to merge it there. But if QE is fine, I'm fine as well.

masayag · 2025-01-08T16:01:29Z

This PR should be merged in milestone-5, changing it to draft to avoid accidental merge

Somehow I thought we wanted this to 1.4. Thanks for the clarification.

I thought so, but since we're in RC for 1.4, I think it's not a good idea to merge it there. But if QE is fine, I'm fine as well.

Ok, so let's merge this one in milestone-5 to reduce instabilities.

jenniferubah · 2025-01-09T15:18:03Z

This PR should be merged in milestone-5, changing it to draft to avoid accidental merge

Somehow I thought we wanted this to 1.4. Thanks for the clarification.

I thought so, but since we're in RC for 1.4, I think it's not a good idea to merge it there. But if QE is fine, I'm fine as well.

Ok, so let's merge this one in milestone-5 to reduce instabilities.

@jordigilh @masayag, please could we split this PR? There are changes pertaining to consistency with source/ sourceName that would resolve FLPATH-1981 and FLPATH-1956 for 1.4 release. And then the other changes could be in the other PR for 1.5

jordigilh · 2025-01-09T15:19:10Z

This PR should be merged in milestone-5, changing it to draft to avoid accidental merge

Somehow I thought we wanted this to 1.4. Thanks for the clarification.

I thought so, but since we're in RC for 1.4, I think it's not a good idea to merge it there. But if QE is fine, I'm fine as well.

Ok, so let's merge this one in milestone-5 to reduce instabilities.

@jordigilh @masayag, please could we split this PR? There are changes pertaining to consistency with source/ sourceName that would resolve FLPATH-1981 and FLPATH-1956 for 1.4 release. And then the other changes could be in the other PR for 1.5

Will do. I'll raise a new PR shortly

jordigilh requested review from gabriel-farache, jenniferubah and masayag December 20, 2024 20:41

jordigilh changed the title ~~Cleanup CRD fields in v1alpha2~~ Cleanup CRD fields in v1alpha2 of Release Candidate related fields Dec 20, 2024

jordigilh changed the title ~~Cleanup CRD fields in v1alpha2 of Release Candidate related fields~~ [1.4] Cleanup CRD fields in v1alpha2 of Release Candidate related fields Dec 20, 2024

jordigilh added the milestone-4 label Dec 20, 2024

masayag reviewed Dec 22, 2024

View reviewed changes

gabriel-farache reviewed Dec 23, 2024

View reviewed changes

masayag reviewed Jan 7, 2025

View reviewed changes

bundle/manifests/orchestrator-operator.clusterserviceversion.yaml Outdated Show resolved Hide resolved

jordigilh added 5 commits January 7, 2025 16:25

Normalized to source variable name

96a2cd6

Removed isReleaseCandidate

3eedd98

Remove rhdh/sonataflowPlatform image field used specifically for RC

f40d2c8

Remove RC fields to allow customize dataIndexImage and jobServiceImag…

f438bad

…e under orchestrator.sonataflowPlatform.jobServiceImage and orchestrator.sonataflowPlatform.dataIndexImage

Removed get-cluster-version helper

6c49567

jordigilh force-pushed the cleanup_values.yaml_v1alpha2 branch from 9928818 to f60b128 Compare January 7, 2025 21:27

jordigilh force-pushed the cleanup_values.yaml_v1alpha2 branch 2 times, most recently from 6bfbf3d to 7a31d8a Compare January 7, 2025 22:26

Moved rhdhOperator.subscription.targetNamespace to rhdhOperator.targe…

d15e518

…tNamespace

jordigilh force-pushed the cleanup_values.yaml_v1alpha2 branch from 7a31d8a to d15e518 Compare January 8, 2025 14:26

jordigilh added milestone-5 and removed milestone-4 labels Jan 8, 2025

jordigilh marked this pull request as draft January 8, 2025 14:43

jordigilh changed the title ~~[1.4] Cleanup CRD fields in v1alpha2 of Release Candidate related fields~~ [1.5] Cleanup CRD fields in v1alpha2 of Release Candidate related fields Jan 8, 2025

	{{- else if (and (.Values.rhdhOperator.enabled .Values.rhdhOperator.targetNamespace )}}
	{{- $namespace = .Values.rhdhOperator.targetNamespace }} # otherwise, use targetNamespace if defined

Conversation

jordigilh commented Dec 20, 2024

Uh oh!

masayag Dec 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jordigilh Jan 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jenniferubah Jan 7, 2025

Choose a reason for hiding this comment

Uh oh!

jenniferubah Jan 7, 2025

Choose a reason for hiding this comment

Uh oh!

jordigilh Jan 7, 2025

Choose a reason for hiding this comment

Uh oh!

jordigilh Jan 7, 2025

Choose a reason for hiding this comment

Uh oh!

jordigilh Jan 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

masayag Jan 8, 2025

Choose a reason for hiding this comment

Uh oh!

masayag Dec 22, 2024

Choose a reason for hiding this comment

Uh oh!

gabriel-farache left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

masayag commented Jan 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jordigilh commented Jan 7, 2025

Uh oh!

jordigilh commented Jan 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jordigilh commented Jan 8, 2025

Uh oh!

jordigilh commented Jan 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

masayag commented Jan 8, 2025

Uh oh!

jordigilh commented Jan 8, 2025

Uh oh!

masayag commented Jan 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jenniferubah commented Jan 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jordigilh commented Jan 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

masayag Dec 22, 2024 •

edited

Loading

jordigilh Jan 7, 2025 •

edited

Loading

jordigilh Jan 8, 2025 •

edited

Loading

gabriel-farache left a comment •

edited

Loading

masayag commented Jan 7, 2025 •

edited

Loading

jordigilh commented Jan 7, 2025 •

edited

Loading

jordigilh commented Jan 8, 2025 •

edited

Loading

masayag commented Jan 8, 2025 •

edited

Loading

jenniferubah commented Jan 9, 2025 •

edited

Loading