HOSTEDCP-2246: [CPOv2 Refactor] Refactor OLM #5663
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@muraee: This pull request references HOSTEDCP-2246 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 sub-task to target the "4.19.0" version, but no target version was set. In 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. |
openshift-ci
bot
added
the
area/control-plane-operator
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: muraee The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
approved
muraee
force-pushed
the
refactor-olm
branch
2 times, most recently
from
b2c6d15 to
2757385
Compare
control-plane-operator/controllers/hostedcontrolplane/v2/olm/collect_profiles/component.go
Show resolved
Hide resolved
| deploymentConfig.ApplyTo(deployment) | ||
| return nil | ||
| } | ||
| c.workloadProvider.ApplyOptionsTo(cpContext, workloadObj, oldWorkloadObj, deploymentConfig) |
There was a problem hiding this comment.
I wouldn't expect we need to pass deploymentConfig through any signaturem specially public
There was a problem hiding this comment.
the problem is there is not generic function for deploymentConfig.ApplyTo si each workloadProvider needs to call the correct function. I'd like to get rid of deploymentConfig in the cpov2 completely once finish the refactors to make the code more readable and explicit.
through any signaturem specially public
technically this should be only used within this package, I can make it the func private
There was a problem hiding this comment.
we could hide this within the deploymentConfig package rather than surface it in this library
switch any(workloadObj).(type) {
case appsv1.Deployment:
// call right function
case stateful:
not a blocker for me
There was a problem hiding this comment.
thanks, I will follow-up with a PR to remove deploymentConfig from v2
|
/lgtm |
openshift-ci
bot
added
lgtm
|
New changes are detected. LGTM label has been removed. |
muraee
force-pushed
the
refactor-olm
branch
2 times, most recently
from
3c5db82 to
4bb3fd2
Compare
muraee
force-pushed
the
refactor-olm
branch
2 times, most recently
from
d08ec36 to
f9630ef
Compare
|
/retest-required |
|
/retest-required |
1 similar comment
|
/retest-required |
|
@muraee: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
/retest-required |
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, use
fixes #<issue_number>(, fixes #<issue_number>, ...)format, where issue_number might be a GitHub issue, or a Jira story:Fixes #
Checklist