OPRUN-4086: Add ClusterCatalog controller #15466
Conversation
openshift-ci
bot
added
component/backend
|
Hi @trgeiger. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with 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. |
|
/ok-to-test |
openshift-ci
bot
added
ok-to-test
|
do not merge until we add tech preview feature flag support to console--I will then wrap all this functionality in that feature flag |
TheRealJon
left a comment
TheRealJon
left a comment
There was a problem hiding this comment.
Just a couple of questions
| // The ClusterCatalog has been found on the cluster, add to or update cache | ||
| baseURL := clusterCatalog.Status.URLs.Base | ||
|
|
||
| if baseURL != "" { |
There was a problem hiding this comment.
Is this Status.URLs.Base a required property in the ClusterCatalog API? If not, should we add a debug-level log message when it's missing?
There was a problem hiding this comment.
I think URLs is not required but if it exists, Base is a required field on a URL. So yeah I'll add some logic for catching that.
- Use klog for logging - Handle missing URLs or Base fields - Check err when creating controller manager
Default to :8081 to avoid collision with existing metrics endpoint at :8080.
trgeiger
force-pushed
the
solo-catalog-controller
branch
from
1e49535 to
0dafc53
Compare
trgeiger
force-pushed
the
solo-catalog-controller
branch
from
0dafc53 to
c3ea325
Compare
|
QE Approver Docs Approver: PX Approver: |
|
@TheRealJon: GitHub didn't allow me to assign the following users: jseseCCS. Note that only openshift members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. 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. |
openshift-ci
bot
added
lgtm
trgeiger
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@trgeiger: This pull request references OPRUN-4086 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 story to target the "4.21.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. |
|
/retest |
|
/label px-approved |
openshift-ci
bot
added
the
px-approved
|
verified the latest code works fine /verified by @yapei |
openshift-ci-robot
added
the
verified
|
@yapei: This PR has been marked as verified by 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. |
jseseCCS
left a comment
jseseCCS
left a comment
There was a problem hiding this comment.
In any of these log messages, we should start them with initial caps ("Problem creating...") and end with a period whenever appropriate.
jseseCCS
left a comment
jseseCCS
left a comment
There was a problem hiding this comment.
Global suggestions: Make sure "R/reconciliation" is spelled correctly everywhere.
Go often doesn't use underscores in test names. Just something to think about.
Make "test-catalog" a constant? >> const testCatalogName = "test-catalog"?
update failed/remove failed >> mock update failed/mock remove failed?
| // Reconcile implements the reconcile.Reconciler interface | ||
| func (r *ClusterCatalogReconciler) Reconcile(ctx context.Context, req reconcile.Request) (reconcile.Result, error) { | ||
| klog.Infof("Reconciling ClusterCatalog %s", req.Name) | ||
| defer klog.Infof("Reconcilation ending for ClusterCatalog %s", req.Name) |
There was a problem hiding this comment.
change to "Reconciliation..." (missing an i after the l).
Also, consider changing tense to match: "Reconciling ended..." OR change line 35 to "Starting reconciliation..." and line 36 to "Ending reconciliation..."
There was a problem hiding this comment.
Opted for the latter, "Starting" and "Ending"
| baseURL := clusterCatalog.Status.URLs.Base | ||
|
|
||
| if baseURL == "" { | ||
| klog.Infof("ClusterCatalog %s Base URL is empty", req.Name) |
There was a problem hiding this comment.
Yep, the field is Base in the API
|
back to you with a few questions/suggestions, @TheRealJon :) |
openshift-ci-robot
removed
the
verified
trgeiger
force-pushed
the
solo-catalog-controller
branch
from
216e791 to
e0d483e
Compare
|
|
the latest log is as follows |
|
/lgtm |
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jseseCCS, TheRealJon, trgeiger 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 |
|
/label docs-approved |
openshift-ci
bot
added
the
docs-approved
|
/retest-required |
|
/retest |
1 similar comment
|
/retest |
|
@trgeiger: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
/verified by @yapei Applying this label. The latest changes were verified, but I think the label was accidentally omitted. |
openshift-ci-robot
added
the
verified
|
@TheRealJon: This PR has been marked as verified by 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. |
9 participants
Adds a controller for reconciling ClusterCatalogs. Currently it uses a dummy "CatalogService" that doesn't actually do anything.
This is a reformatting and cherry-pick of commits in this draft PR so we can land the controller first and iterate on its functionality in a future PR to add the actual CatalogService implementation and front-end/handler logic.
Vendor and dependency updates are in their own commit to make it easier to review the actual code changes.
The work for creating this controller is tracked in OPRUN-4086.