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
OCPBUGS-18348: Add deprecation alert for DeploymentConfig #12968
OCPBUGS-18348: Add deprecation alert for DeploymentConfig #12968
Conversation
@avivtur: This pull request references OCPSTRAT-118 which is a valid jira issue. 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. |
96f817b
to
3db9cfa
Compare
@avivtur: This pull request references CONSOLE-3708 which is a valid jira issue. 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. |
@opayne1 could you please help to formulate the info alerts text here. |
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.
@jhadvig Added a suggestion! Let me know if you would like to discuss more 😄
3db9cfa
to
8a4e32c
Compare
@avivtur: This pull request references CONSOLE-3708 which is a valid jira issue. 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. |
8a4e32c
to
5c55307
Compare
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.
Sorry for not adding these sooner.
5c55307
to
04a8fe5
Compare
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.
/label docs-approved
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.
Thanks @avivtur, PR looks good 👍
/lgtm
/approve
QE Approver:
|
/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.
Just one question. Is peaceful light blue the correct color for a deprecation? It seems on the 'all is well' side of the spectrum, while light red or orange might better signal 'Hey, you'd better take care of this!'
Just questioning-- hopefully our UI SMEs will have already considered this.
@avivtur: This pull request references Jira Issue OCPBUGS-18348, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
/jira refresh |
/label acknowledge-critical-fixes-only |
@jhadvig: This pull request references Jira Issue OCPBUGS-18348, which is invalid:
Comment 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. |
/jira refresh |
@jhadvig: This pull request references Jira Issue OCPBUGS-18348, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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 |
I looked into why the Helm E2e job fails. Here is what I have done so far: When you click in the latest CI job https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_console/12968/pull-ci-openshift-console-master-e2e-gcp-console/1697364072602800128 on artifacts and follow this path to the screenshots, you can see multiple screenshots that shows that the topology crashes with this change: https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openshift_console/12968/pull-ci-openshift-console-master-e2e-gcp-console/1697364072602800128/artifacts/e2e-gcp-console/test/artifacts/gui_test_screenshots/cypress/screenshots/helm-release.feature/ I will check out the PR to investigate why this happened and how this is related to this change. Until then, let's hold this PR please. /hold |
@@ -16,6 +18,11 @@ const SideBarHeading: React.FC<{ element: GraphElement }> = ({ element }) => { | |||
<TopologyActions element={element} /> | |||
</div> | |||
</h1> | |||
{element.getData().resource.kind === DeploymentConfigModel.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.
Hey @avivtur, the crash I mentioned above happens here, when the user selects a Helm Chart.
And the e2e test failed because it imports a Helm Chart (the Nodejs one from the Helm catalog) and then automatically selects the created Helm Chart. And then the error page is shown and Cypress waits to find the elements its looking for.
So the right way is to harden this code so that it doesn't crash.
The problem is that a Helm chart is not a k8s resource and so the getData()
method doesn't return one. (It's mostly saved in a Secret, but there are other options.)
TypeError: Cannot read properties of undefined (reading 'kind')
Component trace:
at SideBarHeading (http://localhost:12968/static/dev-console/code-refs/topology~overview-list-page-f3b1fecca527617e2d8b.js:3145:27)
In this case, resource
is undefined. But in other cases getData()
could also return undefined
, based on the type definition of this GraphElement
.
So I recommend that you add a new variable at the beginning of the component:
const resourceKind = element.getData()?.resource?.kind;
And then using this here:
{element.getData().resource.kind === DeploymentConfigModel.kind && ( | |
{resourceKind === DeploymentConfigModel.kind && ( |
Of course, you can add this optional check also inline.
And FYI: The reason why it also crashes when you load the /topology after that is because we're saving the latest selection.
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.
@jerolimov Hi there 😄
Thank you very much for looking into this issue and for the very detailed explanation 💯
I'll change my code according to it
Signed-off-by: Aviv Turgeman <aturgema@redhat.com>
5b6283f
to
b284167
Compare
Force pushed changes looks good to me 👍 /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.
I tested this locally again and it works fine.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: avivtur, jerolimov, jhadvig 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 |
@avivtur: all tests passed! 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/test-infra repository. I understand the commands that are listed here. |
@avivtur: Jira Issue OCPBUGS-18348: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-18348 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. |
Adding an alert for deprecation of DeploymentConfig.
More info:
Jira issue: https://issues.redhat.com/browse/OCPSTRAT-118
UX design doc: https://docs.google.com/document/d/1FXxHcsC8llE5IEFJmU3RTYDQBP9-dao718pEwAzKGQw/edit
Demo:
Learn more URL:
https://access.redhat.com/documentation/en-us/openshift_container_platform/4.13/html/building_applications/deployments#doc-wrapper
TODO: