Skip to content

Conversation

gouyang
Copy link
Contributor

@gouyang gouyang commented Sep 7, 2021

Note for review:

  1. introduce support/virtualization
  2. improve deleteResource, make it a oneline function
  3. fix flaky while pause a VM

@openshift-ci openshift-ci bot requested review from pcbailey and rhrazdil September 7, 2021 08:30
@openshift-ci openshift-ci bot added the component/kubevirt Related to kubevirt-plugin label Sep 7, 2021
@gouyang gouyang force-pushed the improve_support branch 2 times, most recently from 5c07eec to d64ac89 Compare September 8, 2021 04:33
@gouyang gouyang changed the title add support/virtualization add support/virtualization and improve deleteSource Sep 8, 2021
@gouyang gouyang changed the title add support/virtualization and improve deleteSource add support/virtualization and improve deleteResource Sep 8, 2021
@gouyang gouyang force-pushed the improve_support branch 2 times, most recently from 95e2d05 to 757ba44 Compare September 8, 2021 04:38
@gouyang gouyang changed the title add support/virtualization and improve deleteResource Bug 2002133: add support/virtualization and improve deleteResource Sep 8, 2021
@openshift-ci openshift-ci bot added bugzilla/severity-low Referenced Bugzilla bug's severity is low for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Sep 8, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 8, 2021

@gouyang: This pull request references Bugzilla bug 2002133, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.10.0) matches configured target release for branch (4.10.0)
  • bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @gouyang

In response to this:

Bug 2002133: add support/virtualization and improve deleteResource

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.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 8, 2021

@openshift-ci[bot]: GitHub didn't allow me to request PR reviews from the following users: gouyang.

Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

@gouyang: This pull request references Bugzilla bug 2002133, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.10.0) matches configured target release for branch (4.10.0)
  • bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @gouyang

In response to this:

Bug 2002133: add support/virtualization and improve deleteResource

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.

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.

@gouyang
Copy link
Contributor Author

gouyang commented Sep 8, 2021

/retest

@gouyang
Copy link
Contributor Author

gouyang commented Sep 8, 2021

/retest

1 similar comment
@gouyang
Copy link
Contributor Author

gouyang commented Sep 8, 2021

/retest

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 8, 2021

@gouyang: This pull request references Bugzilla bug 2002133, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.10.0) matches configured target release for branch (4.10.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @gouyang

In response to this:

Bug 2002133: add support/virtualization and improve deleteResource

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.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 8, 2021

@openshift-ci[bot]: GitHub didn't allow me to request PR reviews from the following users: gouyang.

Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

@gouyang: This pull request references Bugzilla bug 2002133, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.10.0) matches configured target release for branch (4.10.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @gouyang

In response to this:

Bug 2002133: add support/virtualization and improve deleteResource

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.

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.

@gouyang
Copy link
Contributor Author

gouyang commented Sep 8, 2021

@pcbailey @yaacov PTAL, thanks.

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 remove the PVC as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, PVC is deleted along with DV.

Copy link
Contributor

Choose a reason for hiding this comment

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

I know this isn't really a part of this PR, but could you please rename this to 'DEFAULT_VALUES'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, will change the name.

Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: K8SKIND => K8S_KINDS

Copy link
Contributor Author

@gouyang gouyang Sep 8, 2021

Choose a reason for hiding this comment

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

How about just use K8S_KIND?

Copy link
Contributor

Choose a reason for hiding this comment

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

That works! Both singular and plural names are used for enums throughout the codebase, but I think you're right that it should be singular.

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought we had a scenario where we'd get an error if we didn't pass false for the ignore-not-found flag? I didn't see any instances where we were passing false, but I wanted to double check with you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I removed the ignoreNotFound = true in the parameters because there isn't any instance pass false to this function.
If there is such scenario, let's add it back by then.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like this idea! My only concern is around naming. We're going to be adding the Virtualization Overview page soon and moving the VMs and Templates pages to their own dedicated pages instead of tabs, so this won't work anymore since there won't be an actual Virtualization page that defaults to the VMs list. What do you think about the following?

  • visitVirtualizationOverview (when it's added)
  • visitVMsList
  • visitVMTemplatesList

Copy link
Contributor Author

Choose a reason for hiding this comment

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

visitVMsList and visitVMTemplatesList looks better, will change the name.

@gouyang gouyang force-pushed the improve_support branch 3 times, most recently from 0fd5d48 to bd13996 Compare September 9, 2021 02:25
@yaacov
Copy link
Member

yaacov commented Sep 9, 2021

/retest

@gouyang gouyang force-pushed the improve_support branch 2 times, most recently from 50b43b7 to 8dcce7a Compare September 9, 2021 08:47
@pcbailey
Copy link
Contributor

pcbailey commented Sep 9, 2021

/test e2e-gcp-console

1 similar comment
@pcbailey
Copy link
Contributor

pcbailey commented Sep 9, 2021

/test e2e-gcp-console

@pcbailey
Copy link
Contributor

pcbailey commented Sep 9, 2021

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 9, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 9, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gouyang, 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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 9, 2021
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit f0f847c into openshift:master Sep 9, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 9, 2021

@gouyang: All pull requests linked via external trackers have merged:

Bugzilla bug 2002133 has been moved to the MODIFIED state.

In response to this:

Bug 2002133: add support/virtualization and improve deleteResource

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 spadgett added this to the v4.10 milestone Sep 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-low Referenced Bugzilla bug's severity is low for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. component/kubevirt Related to kubevirt-plugin lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants