Skip to content

Fix incorrect assumption for bootstrap.iso path#1882

Merged
caglar10ur merged 1 commit intovmware:masterfrom
caglar10ur:iso
Aug 9, 2016
Merged

Fix incorrect assumption for bootstrap.iso path#1882
caglar10ur merged 1 commit intovmware:masterfrom
caglar10ur:iso

Conversation

@caglar10ur
Copy link
Copy Markdown
Contributor

@caglar10ur caglar10ur commented Aug 9, 2016

The ISO path that's generated is based off the display name, not the
actual VM folder path. This pr fixes that also removes the hardcoded
appliance.iso file name.

Fixes #1351 2nd time

@caglar10ur
Fix incorrect assumption for bootstrap.iso path
d8a6d98 
The ISO path that's generated is based off the display name, not the
actual VM folder path. This pr fixes that also removes the hardcoded
appliance.iso file name.

Fixes #1351 2nd time
@caglar10ur
Copy link
Copy Markdown
Contributor Author

caglar10ur commented Aug 9, 2016

Created #1883 for the integration test part

@fdawg4l
Copy link
Copy Markdown
Contributor

fdawg4l commented Aug 9, 2016

LGTM

@caglar10ur
Copy link
Copy Markdown
Contributor Author

caglar10ur commented Aug 9, 2016 

[conur@vir:/opt/go/src/github.com/vmware/vic(iso)] govc datastore.ls ZzZ/*.iso
b.iso
a.iso
[conur@vir:/opt/go/src/github.com/vmware/vic(iso)] govc datastore.download ZzZ/ZzZ.vmx ZzZ.vmx; grep iso ZzZ.vmx
[09-08-16 10:34:30] Downloading... OK
48:ide0:0.fileName = "a.iso"
102:guestinfo./bootstrap_image_path = "[vsanDatastore] 980faa57-97c1-5f89-4578-000c298a9889/b.iso"
[conur@vir:/opt/go/src/github.com/vmware/vic(iso)]

hickeng
hickeng reviewed Aug 9, 2016
View reviewed changes
lib/install/management/appliance.go
return nil, err
}
cdrom = devices.InsertIso(cdrom, fmt.Sprintf("[%s] %s/appliance.iso", conf.ImageStores[0].Host, d.vmPathName))
cdrom = devices.InsertIso(cdrom, fmt.Sprintf("[%s] %s/%s", conf.ImageStores[0].Host, d.vmPathName, settings.ApplianceISO))
Copy link
Copy Markdown
Contributor

@hickeng hickeng Aug 9, 2016

Choose a reason for hiding this comment

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

Should we be using the actual path of the appliance VM rather than composing in this fashion. It'll currently work because we use ImageStores[0] as the applianceVM datastore, but if that ever changes then this will break.

Copy link
Copy Markdown
Contributor Author

@caglar10ur caglar10ur Aug 9, 2016

Choose a reason for hiding this comment

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

conf.ImageStores[0] used everywhere in the vic-machine code so will open a issue to address that if needed

@hickeng
Copy link
Copy Markdown
Contributor

hickeng commented Aug 9, 2016

LGTM other than future proofing.

@caglar10ur caglar10ur merged commit 91a6035 into vmware:master Aug 9, 2016
@caglar10ur caglar10ur mentioned this pull request Aug 9, 2016
Open
@emlin
Copy link
Copy Markdown
Contributor

emlin commented Aug 9, 2016

Caglar, this change is good.
Just wondering if we should custom the iso files in datastore.
I mean we uploaded the file here https://github.com/vmware/vic/blob/master/lib/install/management/create.go#L122, with the filename from local file system, would it be better to use "appliance.iso" and "bootstrap.iso" during uploading, no matter how user name them?
then we could assume them in our code as a constant name.

@caglar10ur caglar10ur deleted the iso branch November 29, 2016 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

cla-not-required

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Incorrect formulation of bootstrap iso path

5 participants

@caglar10ur @fdawg4l @hickeng @emlin @vmwclabot