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

fix typo that undeploys vapp instead of vm while deleting a vm #261

Merged

Conversation

vkhacharia
Copy link
Contributor

Found the pointer in #69 Updated the code and tested locally.

@ghost ghost added the size/XS label Jun 10, 2019
@lvirbalas lvirbalas requested a review from dataclouder June 11, 2019 06:16
@vbauzys
Copy link
Contributor

vbauzys commented Jun 12, 2019

Nice catch, still we need to investigate how it related to vapp.Deploy() part which start from 875 line.

@vbauzys
Copy link
Contributor

vbauzys commented Jun 13, 2019

If everyone would approve I suggest together with this fix remove 873-894 case. I don't see any more value for deploy if don't undepoy vAPP. My testing seem to verify that. I would like other to verify that.

Copy link
Contributor

@dataclouder dataclouder left a comment

Choose a reason for hiding this comment

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

Approved, with the additional request of removing lines 873 to 894.

Tests pass, with several minutes saved in total (three minutes just on the multi-vm test)

@ghost ghost added size/S and removed size/XS labels Jun 14, 2019
Copy link
Contributor

@vbauzys vbauzys left a comment

Choose a reason for hiding this comment

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

Seems good now

Copy link
Collaborator

@lvirbalas lvirbalas left a comment

Choose a reason for hiding this comment

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

Nice!

Copy link
Collaborator

@Didainius Didainius left a comment

Choose a reason for hiding this comment

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

@vkhacharia, thank you for this PR.
Can you add a message to CHANGELOG under IMPROVEMENTS section and let's merge it.

@dataclouder dataclouder merged commit ee539e8 into vmware:master Jun 26, 2019
@vkhacharia
Copy link
Contributor Author

Do I update the changelog and make new pull request?

@Didainius
Copy link
Collaborator

@vkhacharia NP, I will put in a note with the next PR I have in the oven :)
Thank you again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants