Skip to content
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

CNV-19275: Add virtctl to overview #746

Merged
merged 1 commit into from
Jul 11, 2022

Conversation

pcbailey
Copy link
Member

@pcbailey pcbailey commented Jul 7, 2022

📝 Description

This PR adds a link to the overview header that displays a popup with information on downloading virtctl when clicked.

Screenshots

Popup closed

Without the Show getting started resources button
Selection_121

With the Show getting started resources button
Selection_122

Popup open

Without the Show getting started resources button
Selection_124

With the Show getting started resources button
Selection_125

@openshift-ci openshift-ci bot requested review from tnisan and yaacov July 7, 2022 15:09
@openshift-ci openshift-ci bot added the approved This issue is something we want to fix label Jul 7, 2022
@pcbailey pcbailey force-pushed the add-virtctl-to-overview branch 2 times, most recently from 1cff7b4 to 3ceb4ae Compare July 8, 2022 14:27
@pcbailey
Copy link
Member Author

pcbailey commented Jul 8, 2022

@pcbailey pcbailey changed the title Add virtctl to overview CNV-19275: Add virtctl to overview Jul 9, 2022
});

const tools = Array.isArray(cliTools) ? cliTools : [...[cliTools]];
// skipcq: JS-0349
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does it mean?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this skip DeepSource checks? why?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@metalice @glekner Exactly, it's the equivalent of eslint-disable. It was throwing an error because of the K8sResourceKind cast. If I don't cast it I get a type error because spec isn't on K8sResourceCommon. I'm open to a better way to handle that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useK8sWatchResource won't help in this case?

const tools = Array.isArray(cliTools) ? cliTools : [...[cliTools]];
// skipcq: JS-0349
const virtctlObj = tools?.find(
(tool) => (tool as K8sResourceCommon)?.metadata?.name === VIRTCTL_DOWNLOADS,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

u sure K8sResourceCommon and K8sResourceKind are mandatory here as hard casting?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I typically don't hard cast unless I get errors, but I double-checked and was able to remove the hard cast to K8sResourceCommon, but the K8sResourceKind is needed to avoid an error as described in the DeepSource comment above.

@metalice
Copy link
Member

Hi @pcbailey thanks for the PR. I left some small comments :)

@metalice
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Passed code review, ready for merge label Jul 11, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 11, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: metalice, pcbailey

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot merged commit 4cfb797 into kubevirt-ui:main Jul 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved This issue is something we want to fix lgtm Passed code review, ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants