Conversation
Signed-off-by: John Sell <jsell@redhat.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @jsell-rh. Thanks for your PR. I'm waiting for a redhat-developer 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughThe Controller deployment now builds its ChangesController temporary volume
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The change adds the required writable temporary storage without an identified merge-blocking regression. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
| assert.Equal(t, cfg.readinessProbe, retrievedDeployment.Spec.Template.Spec.Containers[0].ReadinessProbe) | ||
| } | ||
|
|
||
| func TestReconcilePromoterControllerDeployment_WritableTmp(t *testing.T) { |
There was a problem hiding this comment.
The check to see if the volume and volume mounts are there should be added to the existing tests that check if the deployment exists instead of making a new test IMO.
| securityContext: buildControllerSecurityContext(), | ||
| livenessProbe: buildControllerLivenessProbe(), | ||
| readinessProbe: buildControllerReadinessProbe(), | ||
| // Git clones and temporary index files need writable storage. |
There was a problem hiding this comment.
Can this be moved to a function for readability.
See buildAPIServerVolumeMounts() and buildAPIServerVolumes().
| assert.Equal(t, cfg.securityContext, retrievedDeployment.Spec.Template.Spec.Containers[0].SecurityContext) | ||
| assert.Equal(t, cfg.livenessProbe, retrievedDeployment.Spec.Template.Spec.Containers[0].LivenessProbe) | ||
| assert.Equal(t, cfg.readinessProbe, retrievedDeployment.Spec.Template.Spec.Containers[0].ReadinessProbe) | ||
| for _, tt := range []struct { |
There was a problem hiding this comment.
I was thinking something more of just adding something like this. No need to change the whole test structure.
assert.Equal(t, cfg.volumeMounts, retrievedDeployment.Spec.Template.Spec.Containers[0].VolumeMounts)
assert.Equal(t, cfg.volumes, retrievedDeployment.Spec.Template.Spec.Volumes)
cjcocokrisp
left a comment
There was a problem hiding this comment.
One more small comment.
What type of PR is this?
/kind bug
What does this PR do / why we need it:
The operator sets
readOnlyRootFilesystem: trueon the GitOps Promoter controller but does not mount writable temporary storage. On OpenShift GitOps 1.22 RC2, the controller starts and passes its probes, but a ChangeTransferPolicy fails when it tries to clone its Git repository:Add an
emptyDirvolume mounted at/tmpto the controller Deployment configuration. This provides space for Git clones and temporary index files while retaining the read-only root filesystem. The existing reconciliation code also adds the volume and mount to Deployments created before this fix and preserves them on later reconciliations.Have you updated the necessary documentation?
No documentation update is required. This restores Git operations without changing the ArgoCD API or configuration requirements.
Which issue(s) this PR fixes:
Observed on an OpenShift GitOps 1.22 RC2 installation. No issue has been filed.
Test acceptance criteria:
How to test changes / Special notes to the reviewer:
The package tests pass. The existing
TestReconcilePromoterControllerDeployment_PromoterEnabledtest retains its structure and adds assertions for the controller volumes and volume mounts. The volume and mount are built bybuildControllerVolumes()andbuildControllerVolumeMounts(), consistent with the API-server configuration.For a cluster test, enable Promoter and create a GitRepository and PromotionStrategy. Verify that the controller pod mounts an
emptyDirat/tmpand that repository cloning proceeds without the read-only filesystem error. This patch has not yet been tested as a built operator image in a cluster.