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

Update vsphere_vm_clone.rb #114

Closed
wants to merge 2 commits into from
Closed

Update vsphere_vm_clone.rb #114

wants to merge 2 commits into from

Conversation

F0RMaTC
Copy link

@F0RMaTC F0RMaTC commented Jun 5, 2014

Adding windows support

Adding windows support
@eklein
Copy link
Contributor

eklein commented Jun 5, 2014

Can you please reformat this with spaces instead of tabs? Most (all?) of this code is written with 2 spaces per tab.

@F0RMaTC
Copy link
Author

F0RMaTC commented Jun 5, 2014

L

Must be the copy and paste, hold on

From: Eli Klein [mailto:notifications@github.com]
Sent: Thursday, June 05, 2014 9:33 PM
To: ezrapagel/knife-vsphere
Cc: Bjorn van de Langenberg
Subject: Re: [knife-vsphere] Update vsphere_vm_clone.rb (#114)

Can you please reformat this with spaces instead of tabs? Most (all?) of this code is written with 2 spaces per tab.


Reply to this email directly or view it on GitHub #114 (comment) . https://github.com/notifications/beacon/5115835__eyJzY29wZSI6Ik5ld3NpZXM6QmVhY29uIiwiZXhwaXJlcyI6MTcxNzYxNTk5OSwiZGF0YSI6eyJpZCI6MzM5NjgzMTJ9fQ==--08f00c481f46193feae02c35295dd82b873b4d72.gif

@eklein
Copy link
Contributor

eklein commented Jun 5, 2014

Sorry my comment may have been confusing. The file you submitted has a lot of tabs in it instead of spaces. If you can use spaces instead of tabs and then simply format the whole file in an editor using 2 spaces per tab, this should clean up nicely.

@F0RMaTC
Copy link
Author

F0RMaTC commented Jun 5, 2014

I already understood it J

It is a mess now, strange that I didn’t see it before.

I will fix it now

From: Eli Klein [mailto:notifications@github.com]
Sent: Thursday, June 05, 2014 10:34 PM
To: ezrapagel/knife-vsphere
Cc: Bjorn van de Langenberg
Subject: Re: [knife-vsphere] Update vsphere_vm_clone.rb (#114)

Sorry my comment may have been confusing. The file you submitted has a lot of tabs in it instead of spaces. If you can use spaces instead of tabs and then simply format the whole file in an editor using 2 spaces per tab, this should clean up nicely.


Reply to this email directly or view it on GitHub #114 (comment) . https://github.com/notifications/beacon/5115835__eyJzY29wZSI6Ik5ld3NpZXM6QmVhY29uIiwiZXhwaXJlcyI6MTcxNzYxOTY0MSwiZGF0YSI6eyJpZCI6MzM5NjgzMTJ9fQ==--4ceef5f9b2bf33d977ecaf8196c519e3b47361b8.gif

@eklein
Copy link
Contributor

eklein commented Jun 5, 2014

No worries, thanks for fixing!

@F0RMaTC
Copy link
Author

F0RMaTC commented Jun 5, 2014

Does this look beter?

From: Eli Klein [mailto:notifications@github.com]
Sent: Thursday, June 05, 2014 10:50 PM
To: ezrapagel/knife-vsphere
Cc: Bjorn van de Langenberg
Subject: Re: [knife-vsphere] Update vsphere_vm_clone.rb (#114)

No worries, thanks for fixing!


Reply to this email directly or view it on GitHub #114 (comment) . https://github.com/notifications/beacon/5115835__eyJzY29wZSI6Ik5ld3NpZXM6QmVhY29uIiwiZXhwaXJlcyI6MTcxNzYyMDYyNSwiZGF0YSI6eyJpZCI6MzM5NjgzMTJ9fQ==--80f85815623dcf217a163f71d70e33abcdc80416.gif

@eklein
Copy link
Contributor

eklein commented Jun 5, 2014

I don't see any changes yet..

@F0RMaTC
Copy link
Author

F0RMaTC commented Jun 5, 2014

I attached the file in the mail ;)

From: Eli Klein [mailto:notifications@github.com]
Sent: Thursday, June 05, 2014 10:58 PM
To: ezrapagel/knife-vsphere
Cc: Bjorn van de Langenberg
Subject: Re: [knife-vsphere] Update vsphere_vm_clone.rb (#114)

I don't see any changes yet..


Reply to this email directly or view it on GitHub #114 (comment) . https://github.com/notifications/beacon/5115835__eyJzY29wZSI6Ik5ld3NpZXM6QmVhY29uIiwiZXhwaXJlcyI6MTcxNzYyMTA2NCwiZGF0YSI6eyJpZCI6MzM5NjgzMTJ9fQ==--8eeafe9f412c4a28cf32cb22322ed44bb8cbb143.gif

Changed tabs to spaces and cleanup
@eklein
Copy link
Contributor

eklein commented Jun 5, 2014

So if you just force push this new file on top of your branch, it should come through in GH. I think it looks good though :)

@eklein
Copy link
Contributor

eklein commented Jun 5, 2014

Looks great. Let me review one last time and then I'll merge.

@F0RMaTC
Copy link
Author

F0RMaTC commented Jun 5, 2014

Cool thnx!

From: Eli Klein [mailto:notifications@github.com]
Sent: Thursday, June 05, 2014 11:39 PM
To: ezrapagel/knife-vsphere
Cc: Bjorn van de Langenberg
Subject: Re: [knife-vsphere] Update vsphere_vm_clone.rb (#114)

Looks great. Let me review one last time and then I'll merge.


Reply to this email directly or view it on GitHub #114 (comment) . https://github.com/notifications/beacon/5115835__eyJzY29wZSI6Ik5ld3NpZXM6QmVhY29uIiwiZXhwaXJlcyI6MTcxNzYyMzU2NCwiZGF0YSI6eyJpZCI6MzM5NjgzMTJ9fQ==--a8ad65d39e81ae2929112a62f479b18cd022f2a1.gif

@eklein
Copy link
Contributor

eklein commented Jun 5, 2014

oops meant to leave this comment here...

I just pulled down your code and installed it to test. It appears you haven't added these new dependencies to the gemspec. Please add any dependencies you need to the gemspec and I can give it another shot once those are in there.

Thanks!

@F0RMaTC
Copy link
Author

F0RMaTC commented Jun 5, 2014

Eli,

Im a bit new on this so if you can help me out that would be great.

The only thing it is relying on is knife-windows. I dont have any chef servers anymore so im unable to test it.

On Jun 5, 2014, at 23:49, Eli Klein notifications@github.com wrote:

oops meant to leave this comment here...

I just pulled down your code and installed it to test. It appears you haven't added these new dependencies to the gemspec. Please add any dependencies you need to the gemspec and I can give it another shot once those are in there.

Thanks!


Reply to this email directly or view it on GitHub.

end
clone_spec.customization = cust_spec
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move this line above if statement as it was done in Issue#101. Otherwise customization plugin cannot change attributes set by customization file.

BTW, on which OS versions did you test this code?

Copy link
Author

Choose a reason for hiding this comment

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

Redhat 6.0 but without cust spec file and windows 2008 r2

On Jun 12, 2014, at 20:14, valldeck notifications@github.com wrote:

In lib/chef/knife/vsphere_vm_clone.rb:

 end
  • clone_spec.customization = cust_spec
    Please move this line above if statement as it was done in Issue#101. Otherwise customization plugin cannot change attributes set by customization file.

BTW, on which OS versions did you test this code?


Reply to this email directly or view it on GitHub.

@F0RMaTC
Copy link
Author

F0RMaTC commented Jul 3, 2014

Any luck yet?

@tim95030
Copy link
Contributor

tim95030 commented Dec 9, 2014

Any plans to merge this in?

@ezrapagel
Copy link
Collaborator

I plan to catch up on merges and branch sometime over the next two weeks. Stay tuned...

On Dec 9, 2014, at 2:36 PM, tim95030 notifications@github.com wrote:

Any plans to merge this in?


Reply to this email directly or view it on GitHub.

@tim95030
Copy link
Contributor

I ended up resolving the merge and doing a major cleanup fixing of this code as such this pull request is encompassed inside my new pull request #137

@radiumx3
Copy link

@ezrapagel 4 months later any sign of life ?

@swalberg
Copy link
Collaborator

I've been helping out with some of the reviews but I've got no way of testing this request.

I can see that the request to add files to the gemspec were never addressed. #137 looks promising but again, no ability for me to test.

@swalberg
Copy link
Collaborator

swalberg commented Jul 1, 2015

Closing as it was incorporated into #137. Thanks for giving us the leg up on this code.

@swalberg swalberg closed this Jul 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants