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.
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
Bug 1780392: Fix hasModel
check for CRD models
#3690
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/hold I could be wrong, but I'll add the hold for now to prevent auto-merge. |
/hold cancel Didn't see the WIP :) |
@@ -27,7 +27,7 @@ export const modelsToMap = (models: K8sKind[]): ImmutableMap<K8sResourceKindRefe | |||
let k8sModels = modelsToMap(_.values(staticModels)); | |||
|
|||
const hasModel = (model: K8sKind) => | |||
k8sModels.has(referenceForModel(model)) || k8sModels.has(model.kind); | |||
k8sModels.has(referenceForModel(model)) || (!model.crd && k8sModels.has(model.kind)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that you got it to work where I failed... I would recommend going with my first implementation and making hasModel
and modelsToMap
use the same logic to figure out the key to store it under.
From Slack:
https://github.com/spadgett/console/blob/586f68d0eaa32defc34df2554b39c5fc226a7d79/frontend/public/module/k8s/k8s-models.ts#L13-L18
I think we need to de-duplicate and reuse this logic. The hasModel should lookup in the same order/reference as the storage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested change:
const modelKey = (model) => {
// TODO: Use `referenceForModel` even for known API objects
return model.crd ? referenceForModel(model) : model.kind;
};
export const modelsToMap = (models: K8sKind[]): ImmutableMap<K8sResourceKindReference, K8sKind> => {
return ImmutableMap<K8sResourceKindReference, K8sKind>().withMutations((map) => {
models.forEach((model) => map.set(modelKey(model), model));
});
};
...
const hasModel = (model: K8sKind) => k8sModels.has(modelKey(model));
That way they are both based off the same value, it looks it up in the same fashion it stores it. No way for it to be confused on reverse lookups.
2b32de0
to
ad3e1ce
Compare
Such familiar code :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can find nothing broken. This solved all of the issues I ran into last time when I was in this area. This appears to have unlocked the ability to properly style Knative Services and Knative Routes.
/lgtm |
hasModel
check for CRD modelshasModel
check for CRD models
The OLM pages work fine clicking through the UI, but this breaks legacy URLs that used |
/hold cancel |
/retest |
2 similar comments
/retest |
/retest |
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/retest
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewballantyne, jeff-phillips-18, spadgett, TheRealJon 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 |
/retest Please review the full test history for this PR and help us cut down flakes. |
5 similar comments
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
@spadgett: All pull requests linked via external trackers have merged. Bugzilla bug 1780392 has been moved to the MODIFIED state. 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 kubernetes/test-infra repository. |
@spadgett: new pull request created: #3718 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 kubernetes/test-infra repository. |
Fixes https://jira.coreos.com/browse/ODC-2125
/assign @andrewballantyne @christianvogt