Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

FLPATH-1856 Enabling monitoring by default in Helm Based Orchestrator operator #422

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

FLPATH-1856 Enabling monitoring by default in Helm Based Orchestrator operator #422

wants to merge 1 commit into from
+43 −14

Conversation

jianrongzhang89
Copy link

@jianrongzhang89 jianrongzhang89 commented Dec 14, 2024 •
edited
Loading

This PR updates the helm based Orchestrator Operator so that the SonataFlowPlatform CR is created with monitoring enabled by default. Users will be able to disable it by setting it to false.

  orchestrator:
    sonataflowPlatform:
      monitoring:
        enabled: true

Tested in RHPDS successfully with the local build of SonataFlow Operator from main branch.

masayag
masayag reviewed Dec 15, 2024
View reviewed changes
Copy link
Collaborator

@masayag masayag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the PR be rebased? It contains changes that aren't seem to be related to monitoring.

bundle/manifests/orchestrator-operator.clusterserviceversion.yaml Outdated
@@ -15,9 +15,15 @@ metadata:
"enabled": false,
"namespace": ""
},
"networkPolicy": {
"rhdhNamespace": "rhdh-operator"
},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be part of this PR?

Copy link
Author

@jianrongzhang89 jianrongzhang89 Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be part of this PR?

Cleaned up :)

Copy link
Collaborator

@jordigilh jordigilh Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see what happened. The problem is that we no longer generate the bundle as part of the build because Konflux forces us to run hermetic builds and we can't retrieve opm at building time. So we're left with sed/awk to make changes on image and version values. And when we have additional changes like new fields, those are not populated to the bundle files. But if we run the make bundle, then these fields now show up in the bundle like something new we changed.

Let me raise a PR now and fix this before yours is merged, then your PR won't have these artifacts.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened #428 that runs make bundle in the existing code. Once it is merged, @jianrongzhang89 please rebase and update the PR, it should have less changes due to running make bundle on your side

@jianrongzhang89 jianrongzhang89 force-pushed the monitoring branch 2 times, most recently from 25151de to 5cc73c8 Compare December 16, 2024 23:19
@jordigilh
Copy link
Collaborator

Should the PR be rebased? It contains changes that aren't seem to be related to monitoring.

I've added a comment to explain why these artifacts exists. In a nutshell, yes it should be rebased but only after my PR #147 is merged, which contains the changes that are side effects by Jeff running make bundle in his environment and pushing the generated changes in the bundle/ directory.

@jenniferubah
Copy link
Collaborator

Also, merged the CRD v1alpha2 PR which was blocking this PR. @jianrongzhang89 this should be good to go now whenever it's ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jordigilh jordigilh jordigilh left review comments

@masayag masayag masayag left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jianrongzhang89 @jordigilh @jenniferubah @masayag