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 1897354: make CRDCard display consistent with tabs #7393
Conversation
@rhamilto: An error was encountered adding this pull request to the external tracker bugs for bug 1897354 on the Bugzilla server at https://bugzilla.redhat.com:
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. |
@rhamilto: This pull request references Bugzilla bug 1897354, which is valid. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
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. |
It occurs to me we should decouple the refresh message from the empty state and show it any time the CSV is recently created, but I'll wait on the design... |
@@ -765,10 +766,15 @@ const crdCardRowStateToProps = ({ k8s }, { crdDescs }) => { | |||
export const CRDCardRow = connect(crdCardRowStateToProps)((props: CRDCardRowProps) => { | |||
const internalObjects = getInternalObjects(props.csv); | |||
const crds = props.crdDescs?.filter(({ name }) => !isInternalObject(internalObjects, name)); | |||
// if the CSV was created within the last 5 minutes (300000 milliseconds) | |||
const recentlyCreated = moment(Date.now()).diff(props.csv.metadata.creationTimestamp) < 300000; |
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.
Isn't 30,000ms == 30s? I would use 5 * 60 * 1000
.
I wonder if we should check the date at all since we could have clock drift between the client and server. I think we should a similar message elsewhere in the UI without the date check.
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.
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.
@rhamilto Disregard, I missed a 0
:)
@rhamilto: This pull request references Bugzilla bug 1897354, which is valid. 3 validation(s) were run on this bug
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. |
3 similar comments
@rhamilto: This pull request references Bugzilla bug 1897354, which is valid. 3 validation(s) were run on this bug
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. |
@rhamilto: This pull request references Bugzilla bug 1897354, which is valid. 3 validation(s) were run on this bug
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. |
@rhamilto: This pull request references Bugzilla bug 1897354, which is valid. 3 validation(s) were run on this bug
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. |
/retest |
@rhamilto I like it 👍, just curious: Would the 'Create instance' button appear when the CRD is actually created then? (The same time the Foo tab stops saying "The server doesn't have a resource type Foo. Try refreshing the page if it was recently added." ?) Would the user need to refresh the Details for that 'create instance' button to appear? |
Hi @itsptk. They should, although there might be a little bit of a delay for normal users. If it doesn't, I'd consider that a bug. |
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhamilto, spadgett 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 |
Yes, the create button should appear automatically. I haven't tested it, but in looking at the code, it should "React". |
@rhamilto: All pull requests linked via external trackers have merged: Bugzilla bug 1897354 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. |
We were filtering out CRDs from the Provided API cards list if the model for the CRD didn't exist; this created a discrepancy between what the cards listed and what the tabs listed. This aligns the cards with the tabs, and both now reflect what's in the CSV YAML.
Reproducer:
add a fake
spec.customresourcedefinitions.owned
to the CSV.Before:
After: