support OVF file upload besides ova #323

guo1017138 · 2020-07-16T02:25:21Z

close #322
The added logic will check the file is ovf or not. if it is OVF, will skip unpack step and will check if it depend disk exists or not.
Signed-off-by: Guo, Larry (NSB - CN/Qingdao) [email protected]

IMPORTANT

To help us process your pull request efficiently, please follow the
guidelines shown below.

A Pull Request should be associated with an Issue

We wish to have discussions in Issues. A single issue may be targeted by multiple PRs.
If you're offering a new feature or fixing anything, we'd like to know beforehand in Issues,
and potentially we'll be able to point development in a particular direction.

We accept PRs without associated issues provided the change is sufficiently evident
from the commit message. If you have typos or simple bug fixes go for it.

Description

Related issue: (<URL or #NUMBER of your Issue>)

(Required) Short description of changes in the PR subject
(Required) Detailed description of changes include tests and
documentation. If the pull request contains multiple commits with
detailed messages, refer to those instead
(Optional) Names of reviewers using @ sign + name

vmwclabot · 2020-07-16T02:25:41Z

@guo1017138, you must sign our contributor license agreement before your changes are merged. Click here to sign the agreement. If you are a VMware employee, read this for further instruction.

vmwclabot · 2020-07-16T17:26:09Z

@guo1017138, VMware has approved your signed contributor license agreement.

vbauzys · 2020-07-20T13:00:25Z

Thank you for your improvements. To fully review it and accept we require new functionality to have tests.
It would be nice to have test which verifies OVF uppload. We can have in github folder with files in test-resource for it. Would be nice if files would be as small as possible. Similar we have with OVA.

guo1017138 · 2020-07-23T05:50:35Z

I will add test case ASAP. Thanks.

guo1017138 · 2020-07-27T07:10:00Z

@vbauzysvmware , I've add test case for OVF file (contents from untar ova), please help to review again and give comments if any

vbauzys

Overall looks good, test passed.
One issue is that after test run, test resources files are missing:

Changes not staged for commit:
  (use "git add/rm <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	deleted:    ../test-resources/test_vapp_template_ovf/descriptor.ovf
	deleted:    ../test-resources/test_vapp_template_ovf/vm-20b41fa7-3212-4faf-9295-045d0bc49783-disk-0.vmdk

After these is fixed, I all for other to review/approve.

govcd/catalog.go

guo1017138 · 2020-08-03T01:56:56Z

Overall looks good, test passed.
One issue is that after test run, test resources files are missing:
Changes not staged for commit:
  (use "git add/rm <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	deleted:    ../test-resources/test_vapp_template_ovf/descriptor.ovf
	deleted:    ../test-resources/test_vapp_template_ovf/vm-20b41fa7-3212-4faf-9295-045d0bc49783-disk-0.vmdk
After these is fixed, I all for other to review/approve.

Add logic check, if this is origin OVF folder directly, won't delete. If it is not OVF folder(means extracted folder from OVA), keep old logic to delete
Add fixed the suggestion changes.
Please help to review again.

vbauzys · 2020-08-03T09:41:32Z

Overall looks good, test passed.
One issue is that after test run, test resources files are missing:
Changes not staged for commit:
  (use "git add/rm <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	deleted:    ../test-resources/test_vapp_template_ovf/descriptor.ovf
	deleted:    ../test-resources/test_vapp_template_ovf/vm-20b41fa7-3212-4faf-9295-045d0bc49783-disk-0.vmdk
After these is fixed, I all for other to review/approve.
Add logic check, if this is origin OVF folder directly, won't delete. If it is not OVF folder(means extracted folder from OVA), keep old logic to delete
Add fixed the suggestion changes.
Please help to review again.

Thank you.
Verified with 9.5 and 10.1 versions.

Only changelog input is needed now and from my side everything is good. @dataclouder @lvirbalas @Didainius - please review also.

lvirbalas

Hello,

Please add a detailed message to CHANGELOG.md :)

guo1017138 · 2020-08-04T01:27:33Z

Thank you all for the review. change log is added in my latest commit.

govcd/catalog.go

lvirbalas

Small suggestion in-line.

BTW, please commit changes in a separate commit. We are squashing them before merging the PR either way, and it's only possible to understand what was changed after a discussion when separate commits are used.

CHANGELOG.md

lvirbalas

@vbauzysvmware confirmed that pulling in this go-vcloud-director build Terraform provider is able to upload OVFs without changes. Thanks for contribution! Waiting for review from @Didainius and @dataclouder next.

Didainius

Thanks

dataclouder

Overall, it looks good.
There is an error in the test that needs to be fixed.
Apart from that, I would like to know the provenance of the test OVF (for future reference)

govcd/catalog_test.go

govcd/api.go

dataclouder

I am waiting for the info on the testing OVF provenance.
Another thing that worries me is that some tests are failing, such as

Running: TestVCD.Test_UploadOvf_ShowUploadProgress_works
catalog_test.go:230:
    check.Assert(string(result), Matches, ".*Upload progress 100.00%")
... value string = "Waiting...\rUpload progress 0.00%\rUpload progress 100.00%\n"
... regex string = ".*Upload progress 100.00%"

FAIL: catalog_test.go:202: TestVCD.Test_UploadOvf_ShowUploadProgress_works

I wasn't able to find the root cause. However, using the repository from July 15 the failing tests succeed, and fails with the current HEAD.

govcd/upload.go

Signed-off-by: Guo, Larry (NSB - CN/Qingdao) <[email protected]>

Support upload ovf which has ovf:size specified for each dependency file ShowUploadProgress can exit whenever task stop, such like cancel on GUI Return status directly for reponse code 400 and 416 due to no body Signed-off-by: Guo, Larry (NSB - CN/Qingdao) <[email protected]>

ovaPath is a variable now instead of a hardcode checking Signed-off-by: Guo, Larry (NSB - CN/Qingdao) <[email protected]>

Signed-off-by: Guo, Larry (NSB - CN/Qingdao) <[email protected]>

guo1017138 · 2020-08-20T01:05:33Z

Rebase done to resolve conflict.

Didainius · 2020-08-24T07:37:13Z

@guo1017138 ,
Thanks for your PR. This is now merged and tagged v2.9.0-alpha.3 of you need to pick it up immediately.

lvirbalas requested a review from vbauzys July 20, 2020 10:19

guo1017138 force-pushed the ovfadd branch from 1f8284c to a525920 Compare July 27, 2020 07:04

vbauzys suggested changes Jul 31, 2020

View reviewed changes

govcd/catalog.go Outdated Show resolved Hide resolved

govcd/catalog.go Outdated Show resolved Hide resolved

guo1017138 force-pushed the ovfadd branch 2 times, most recently from 7752a7a to 7605725 Compare August 3, 2020 01:51

guo1017138 requested a review from vbauzys August 3, 2020 01:54

lvirbalas requested changes Aug 3, 2020

View reviewed changes

guo1017138 force-pushed the ovfadd branch from 7605725 to 55b3b04 Compare August 4, 2020 01:21

lvirbalas requested changes Aug 4, 2020

View reviewed changes

govcd/catalog.go Show resolved Hide resolved

lvirbalas requested review from dataclouder and Didainius August 4, 2020 10:28

vbauzys approved these changes Aug 4, 2020

View reviewed changes

guo1017138 force-pushed the ovfadd branch from 55b3b04 to 48633c3 Compare August 5, 2020 01:11

lvirbalas requested changes Aug 5, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

lvirbalas approved these changes Aug 6, 2020

View reviewed changes

guo1017138 mentioned this pull request Aug 13, 2020

Upload OVF enhancement for known issues #328

Closed

Didainius approved these changes Aug 13, 2020

View reviewed changes

dataclouder suggested changes Aug 16, 2020

View reviewed changes

govcd/catalog_test.go Outdated Show resolved Hide resolved

guo1017138 requested a review from dataclouder August 17, 2020 01:20

guo1017138 force-pushed the ovfadd branch from ab8ae13 to aae0667 Compare August 17, 2020 01:41

dataclouder reviewed Aug 17, 2020

View reviewed changes

govcd/api.go Outdated Show resolved Hide resolved

guo1017138 requested a review from dataclouder August 17, 2020 06:31

dataclouder suggested changes Aug 17, 2020

View reviewed changes

govcd/upload.go Outdated Show resolved Hide resolved

guo1017138 requested a review from dataclouder August 18, 2020 01:42

Guo, Larry (NSB - CN/Qingdao) added 6 commits August 20, 2020 08:56

Add OVF file upload capability

c5e534f

Signed-off-by: Guo, Larry (NSB - CN/Qingdao) <[email protected]>

Change description in changelog

a8d8624

enhance skipWhenOvaPathMissing

416c1c7

ovaPath is a variable now instead of a hardcode checking Signed-off-by: Guo, Larry (NSB - CN/Qingdao) <[email protected]>

return reponse status in header is no body in response

daa17e1

Signed-off-by: Guo, Larry (NSB - CN/Qingdao) <[email protected]>

Remove unnecessary file change for current topic

ea52435

Signed-off-by: Guo, Larry (NSB - CN/Qingdao) <[email protected]>

guo1017138 force-pushed the ovfadd branch from e83ff63 to ea52435 Compare August 20, 2020 01:00

dataclouder approved these changes Aug 24, 2020

View reviewed changes

Didainius merged commit bc933b6 into vmware:master Aug 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support OVF file upload besides ova #323

support OVF file upload besides ova #323

guo1017138 commented Jul 16, 2020

vmwclabot commented Jul 16, 2020

vmwclabot commented Jul 16, 2020

vbauzys commented Jul 20, 2020

guo1017138 commented Jul 23, 2020

guo1017138 commented Jul 27, 2020

vbauzys left a comment

guo1017138 commented Aug 3, 2020

vbauzys commented Aug 3, 2020

lvirbalas left a comment

guo1017138 commented Aug 4, 2020

lvirbalas left a comment

lvirbalas left a comment

Didainius left a comment

dataclouder left a comment

dataclouder left a comment

guo1017138 commented Aug 20, 2020

Didainius commented Aug 24, 2020

support OVF file upload besides ova #323

support OVF file upload besides ova #323

Conversation

guo1017138 commented Jul 16, 2020

IMPORTANT

A Pull Request should be associated with an Issue

Description

vmwclabot commented Jul 16, 2020

vmwclabot commented Jul 16, 2020

vbauzys commented Jul 20, 2020

guo1017138 commented Jul 23, 2020

guo1017138 commented Jul 27, 2020

vbauzys left a comment

Choose a reason for hiding this comment

guo1017138 commented Aug 3, 2020

vbauzys commented Aug 3, 2020

lvirbalas left a comment

Choose a reason for hiding this comment

guo1017138 commented Aug 4, 2020

lvirbalas left a comment

Choose a reason for hiding this comment

lvirbalas left a comment

Choose a reason for hiding this comment

Didainius left a comment

Choose a reason for hiding this comment

dataclouder left a comment

Choose a reason for hiding this comment

dataclouder left a comment

Choose a reason for hiding this comment

guo1017138 commented Aug 20, 2020

Didainius commented Aug 24, 2020