OSAC-1624: remove eda - #302
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:
WalkthroughThis PR disables the EDA component in AAP instance configuration by setting eda.disabled to true and removing eda.hostname bootstrap entries from values.yaml and values-example.yaml. Documentation in the Helm deployment guide and example comments is updated to remove EDA references, leaving controller and gateway components. ChangesDisable EDA and update docs
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ 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 |
|
Hi @htayrie-rh. Thanks for your PR. I'm waiting for a osac-project member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@htayrie-rh: This pull request references OSAC-1624 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
|
@htayrie-rh: This pull request references OSAC-1624 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
charts/osac/values-example.yaml (1)
219-223: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep the example values aligned with the EDA removal.
values-example.yamlstill haseda.disabled: false, so anyone starting from this file will re-enable EDA even though the defaults and docs now say OSAC only needs the controller. Set this totruehere as well, or the example will undo the PR's intended behavior.Proposed fix
controller: disabled: false eda: - disabled: false + disabled: true hub: disabled: true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@charts/osac/values-example.yaml` around lines 219 - 223, The example values still enable EDA in the sample config, which conflicts with the OSAC-only defaults. Update the EDA entry in values-example.yaml so it matches the controller-only setup used by the chart defaults and docs, using the existing controller/eda keys to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@charts/osac/values-example.yaml`:
- Around line 219-223: The example values still enable EDA in the sample config,
which conflicts with the OSAC-only defaults. Update the EDA entry in
values-example.yaml so it matches the controller-only setup used by the chart
defaults and docs, using the existing controller/eda keys to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: df7b6e62-d84c-4bac-8bc0-32d51e5cd835
📒 Files selected for processing (3)
charts/osac/values-example.yamlcharts/osac/values.yamldocs/helm-deployment-guide.md
32369ca to
8cd7916
Compare
8cd7916 to
b611dc2
Compare
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)
charts/osac/values-example.yaml (1)
219-227: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winExample file contradicts its own updated comment.
Comment now says "Only controller is needed for OSAC" but
eda.disabledon line 223 is stillfalse, unlikecharts/osac/values.yamlwhereeda.disabledwas flipped totrue. This leaves the example inconsistent with the stated guidance and the real config.🐛 Proposed fix
eda: - disabled: false + disabled: true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@charts/osac/values-example.yaml` around lines 219 - 227, The OSAC example values are inconsistent with the updated guidance in the AAP components block. Update the `values-example.yaml` example so it matches `values.yaml` and the comment “Only controller is needed for OSAC” by setting `eda.disabled` to true in the AAP components section. Keep the example aligned with the `controller`, `eda`, `hub`, and `lightspeed` entries so the documented default configuration is consistent.charts/osac/values.yaml (1)
95-103: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRemove the stale
bootstrap.eda.hostnameschema entry.charts/osac/values.schema.jsonstill defines that field underbootstrap.eda, so old overrides remain valid even thoughcharts/osac/values.yamlno longer uses it. Keep the schema in sync with the new values shape.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@charts/osac/values.yaml` around lines 95 - 103, The bootstrap values shape no longer includes bootstrap.eda.hostname, but charts/osac/values.schema.json still accepts it. Update the schema definition to remove the stale bootstrap.eda.hostname entry and keep the bootstrap section aligned with the current values fields, using the schema symbols under bootstrap and eda to locate the outdated property.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@base/osac-aap`:
- Line 1: Update the AAP deployment pins to match the new base/osac-aap
submodule commit: replace the old ghcr.io/osac-project/osac-aap:sha-140bdb4
image tag and AAP_PROJECT_GIT_BRANCH=140bdb4fbfd81dd4000e583e3dfab8ad8b1e412a
everywhere they are set, including the caas-ci and vmaas-ci values and the
matching overlay kustomizations, so all AAP references stay aligned with
aa0404d164248420b0dabf02056581f016455102.
In `@scripts/refresh-after-snapshot.py`:
- Around line 535-537: The oc label call in the adoption flow is not checked, so
a failed labeling step is silent and leaves the resource un-adopted. Update the
label step near the existing oc("annotate", ...) logic in
refresh-after-snapshot.py to inspect the oc("label", ...) result from the same
adoption path and emit a warning or error when it fails, using the existing
resource and config.namespace context for clear reporting.
---
Outside diff comments:
In `@charts/osac/values-example.yaml`:
- Around line 219-227: The OSAC example values are inconsistent with the updated
guidance in the AAP components block. Update the `values-example.yaml` example
so it matches `values.yaml` and the comment “Only controller is needed for OSAC”
by setting `eda.disabled` to true in the AAP components section. Keep the
example aligned with the `controller`, `eda`, `hub`, and `lightspeed` entries so
the documented default configuration is consistent.
In `@charts/osac/values.yaml`:
- Around line 95-103: The bootstrap values shape no longer includes
bootstrap.eda.hostname, but charts/osac/values.schema.json still accepts it.
Update the schema definition to remove the stale bootstrap.eda.hostname entry
and keep the bootstrap section aligned with the current values fields, using the
schema symbols under bootstrap and eda to locate the outdated property.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: d8e91ce6-6db7-4bec-a47a-ab4b2aecce50
📒 Files selected for processing (8)
base/osac-aapcharts/osac/values-example.yamlcharts/osac/values.yamldocs/helm-deployment-guide.mdscripts/refresh-after-snapshot.pyscripts/setup.shvalues/caas-ci/values.yamlvalues/vmaas-ci/values.yaml
💤 Files with no reviewable changes (1)
- scripts/setup.sh
d016def to
f0af46c
Compare
35b0478 to
98f9965
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)
charts/osac/values-example.yaml (1)
220-228: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winExample still enables EDA despite the updated comment.
The comment now reads "Only controller is needed for OSAC," but
eda.disabledis stillfalseright below it (Line 224). This contradicts both the comment and the real default set incharts/osac/values.yaml(eda.disabled: true). Update the example value to match.🔧 Proposed fix
controller: disabled: false eda: - disabled: false + disabled: true hub: disabled: true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@charts/osac/values-example.yaml` around lines 220 - 228, The OSAC example values are inconsistent with the updated comment because `eda.disabled` is still set to false even though only `controller` should be enabled and the real default in `values.yaml` is true. Update the example in `values-example.yaml` so the `eda` entry matches the intended OSAC configuration by disabling EDA, and keep the surrounding `controller`, `hub`, and `lightspeed` settings aligned with that example.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@overlays/osac-integration/kustomization.yaml`:
- Around line 44-46: The AAP_PROJECT_GIT_URI literal in the osac-integration
kustomization is wrapped in quotes, which causes the quotes to become part of
the generated secret value. Update the literal in the same way as the sibling
overlays (caas-ci and vmaas-ci) so the value is set as the raw URI string, and
keep the change localized to the kustomize literals list that includes
AAP_EE_IMAGE and AAP_PROJECT_GIT_BRANCH.
---
Outside diff comments:
In `@charts/osac/values-example.yaml`:
- Around line 220-228: The OSAC example values are inconsistent with the updated
comment because `eda.disabled` is still set to false even though only
`controller` should be enabled and the real default in `values.yaml` is true.
Update the example in `values-example.yaml` so the `eda` entry matches the
intended OSAC configuration by disabling EDA, and keep the surrounding
`controller`, `hub`, and `lightspeed` settings aligned with that example.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: d31d2330-2dc5-4e5a-bb8d-befb119863dd
📒 Files selected for processing (10)
base/kustomization.yamlbase/osac-aapcharts/osac/values-example.yamlcharts/osac/values.yamldocs/helm-deployment-guide.mdoverlays/caas-ci/kustomization.yamloverlays/osac-integration/kustomization.yamloverlays/vmaas-ci/kustomization.yamlvalues/caas-ci/values.yamlvalues/vmaas-ci/values.yaml
| - AAP_EE_IMAGE=ghcr.io/osac-project/osac-aap:sha-8781497 | ||
| - AAP_PROJECT_GIT_URI="https://github.com/osac-project/osac-aap" | ||
| - AAP_PROJECT_GIT_BRANCH=77cd796c54d4dbe35dd8ea7ace54dfebb17e9fee | ||
| - AAP_PROJECT_GIT_BRANCH=8781497beb7dac0267d0bb2f2b77b7cca34ab337 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Stray quotes around AAP_PROJECT_GIT_URI will leak into the secret value.
Sibling overlays (caas-ci, vmaas-ci) set this literal unquoted: AAP_PROJECT_GIT_URI=https://github.com/osac-project/osac-aap. Here it's AAP_PROJECT_GIT_URI="https://github.com/osac-project/osac-aap". Kustomize literal sources split only on the first =; the surrounding " chars are not YAML/shell-stripped mid-string, so they become part of the secret value itself, producing a malformed git URI for the config-as-code bootstrap job.
🔧 Proposed fix
- - AAP_PROJECT_GIT_URI="https://github.com/osac-project/osac-aap"
+ - AAP_PROJECT_GIT_URI=https://github.com/osac-project/osac-aap📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - AAP_EE_IMAGE=ghcr.io/osac-project/osac-aap:sha-8781497 | |
| - AAP_PROJECT_GIT_URI="https://github.com/osac-project/osac-aap" | |
| - AAP_PROJECT_GIT_BRANCH=77cd796c54d4dbe35dd8ea7ace54dfebb17e9fee | |
| - AAP_PROJECT_GIT_BRANCH=8781497beb7dac0267d0bb2f2b77b7cca34ab337 | |
| - AAP_EE_IMAGE=ghcr.io/osac-project/osac-aap:sha-8781497 | |
| - AAP_PROJECT_GIT_URI=https://github.com/osac-project/osac-aap | |
| - AAP_PROJECT_GIT_BRANCH=8781497beb7dac0267d0bb2f2b77b7cca34ab337 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@overlays/osac-integration/kustomization.yaml` around lines 44 - 46, The
AAP_PROJECT_GIT_URI literal in the osac-integration kustomization is wrapped in
quotes, which causes the quotes to become part of the generated secret value.
Update the literal in the same way as the sibling overlays (caas-ci and
vmaas-ci) so the value is set as the raw URI string, and keep the change
localized to the kustomize literals list that includes AAP_EE_IMAGE and
AAP_PROJECT_GIT_BRANCH.
|
/ok-to-test |
|
new snapshot for e2e is required in order to pass tests @omer-vishlitzky |
Signed-off-by: Haim Tayrie <htayrie@htayrie-thinkpadt14gen5.raanaii.csb>
Signed-off-by: Haim Tayrie <htayrie@htayrie-thinkpadt14gen5.raanaii.csb>
Signed-off-by: Haim Tayrie <htayrie@htayrie-thinkpadt14gen5.raanaii.csb>
3032341 to
4a80204
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)
charts/osac/values-example.yaml (1)
220-224: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
eda.disabledis stillfalsein the example file — inconsistent with the PR intent.Line 220 comment says "Only controller is needed for OSAC" and the main
values.yamlsetseda.disabled: true, but line 224 still haseda.disabled: false. This means anyone copying the example file as their starting point would inadvertently enable EDA, contradicting the PR's goal of disabling it.🔧 Proposed fix
# AAP components to enable. Only controller is needed for OSAC. controller: disabled: false eda: - disabled: false + disabled: true hub:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@charts/osac/values-example.yaml` around lines 220 - 224, The OSAC example values still enable EDA even though the example comment and the main values configuration indicate only controller should be enabled. Update the example configuration in values-example.yaml so the eda.disabled setting matches the intended OSAC defaults, and verify the controller/eda block stays consistent with the main values.yaml and the AAP components example.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@charts/osac/values.yaml`:
- Line 86: The values schema still exposes the stale bootstrap.eda.hostname
entry even though values.yaml no longer defines it; update the schema to remove
that property and keep the bootstrap/eda section aligned with the current chart
values. Locate the bootstrap.eda object in values.schema.json and delete the
hostname field so validation and generated docs match the actual values
structure.
---
Outside diff comments:
In `@charts/osac/values-example.yaml`:
- Around line 220-224: The OSAC example values still enable EDA even though the
example comment and the main values configuration indicate only controller
should be enabled. Update the example configuration in values-example.yaml so
the eda.disabled setting matches the intended OSAC defaults, and verify the
controller/eda block stays consistent with the main values.yaml and the AAP
components example.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 2a9c9d63-33c5-4b9d-8c09-e5ab2bfb5184
📒 Files selected for processing (3)
charts/osac/values-example.yamlcharts/osac/values.yamldocs/helm-deployment-guide.md
| disabled: false | ||
| eda: | ||
| disabled: false | ||
| disabled: true |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if values.schema.json still references eda.hostname
rg -n "eda\.hostname\|eda.*hostname" charts/osac/values.schema.json -C3Repository: osac-project/osac-installer
Length of output: 165
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## values.yaml\n'
rg -n "eda|hostname|bootstrap" charts/osac/values.yaml -C 3
printf '\n## values.schema.json\n'
rg -n "eda|hostname|bootstrap" charts/osac/values.schema.json -C 3
printf '\n## all repo references to eda.hostname\n'
rg -n "eda\.hostname" .Repository: osac-project/osac-installer
Length of output: 3250
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## charts/osac/values.yaml\n'
sed -n '70,110p' charts/osac/values.yaml | cat -n
printf '\n## charts/osac/values.schema.json\n'
sed -n '1,220p' charts/osac/values.schema.json | cat -n
printf '\n## repo-wide references to eda hostname keys\n'
rg -n '"hostname"|eda\.hostname|eda.*hostname' charts/osac -C 2Repository: osac-project/osac-installer
Length of output: 10635
Remove the stale bootstrap.eda.hostname schema entry. charts/osac/values.schema.json:453-460 still defines bootstrap.eda.hostname, but charts/osac/values.yaml no longer exposes that value. Keep the schema aligned with the chart values to avoid misleading validation/docs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@charts/osac/values.yaml` at line 86, The values schema still exposes the
stale bootstrap.eda.hostname entry even though values.yaml no longer defines it;
update the schema to remove that property and keep the bootstrap/eda section
aligned with the current chart values. Locate the bootstrap.eda object in
values.schema.json and delete the hostname field so validation and generated
docs match the actual values structure.
Source: Coding guidelines
|
/override ci/prow/e2e-vmaas |
|
@omer-vishlitzky: Overrode contexts on behalf of omer-vishlitzky: ci/prow/e2e-vmaas DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: htayrie-rh, omer-vishlitzky The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4 participants
Summary by CodeRabbit
Documentation
Bug Fixes