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

Adding JetBrains vSphere plugin support #162

Closed
wants to merge 2 commits into from
Closed

Conversation

thor
Copy link

@thor thor commented Jan 10, 2019

Adds support for the types vsphere-iso and vsphere-clone from the open source JetBrains vSphere plugin.

Issue

There was no support for the vSphere plugin.

List of Changes Proposed

Adds support for the vsphere-iso and vsphere-clone types to facilitate packerlicious support for these too.

Testing Evidence

I've used it myself to build both ISO and clone templates, but I'll let the CI speak for itself.

  • Add tests for cloning
  • Add tests for ISOs

One note may be that it is not Python 2 compatible, but I can fix that if you'd like. 👍

@coveralls
Copy link

coveralls commented Jan 10, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 2a668fd on thor:master into bd8c977 on mayn:master.

@thor thor changed the title Adding JetBrains vSphere plugin support [WIP] Adding JetBrains vSphere plugin support Jan 10, 2019
@thor thor changed the title [WIP] Adding JetBrains vSphere plugin support Adding JetBrains vSphere plugin support Jan 14, 2019
@thor
Copy link
Author

thor commented Jan 14, 2019

I've updated it to include tests. Can we get this merged, @mayn?

@thor thor force-pushed the master branch 2 times, most recently from 6755338 to 847ef44 Compare January 14, 2019 11:09
Copy link
Owner

@mayn mayn left a comment

Choose a reason for hiding this comment

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

@thor, thanks for taking the time to make this contribution.
I have just a few questions.

src/thirdparty/troposphere/__init__.py Show resolved Hide resolved
@@ -0,0 +1,146 @@
"""
Copyright 2018 Thor K. Hoegaas <thor at roht no>
Copy link
Owner

Choose a reason for hiding this comment

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

i think adding this will potentially complicate things for me later.

Copy link
Author

Choose a reason for hiding this comment

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

I would hope not; the copyright notice does not change anything for your current license unless you want to change your license to an incompatible one at a later point?

@mayn mayn added the awaiting-reply hey! get back to me on this label Jan 26, 2019
@mayn
Copy link
Owner

mayn commented Jan 31, 2019

@thor looking to hear back from you, left some comments on your PR.
Thanks!

@thor
Copy link
Author

thor commented Jan 31, 2019

@mayn my apologies for the late reply; hats have been burning! I've addressed your first question, and await your response regarding the second. ☺️

@mayn
Copy link
Owner

mayn commented Feb 4, 2019

not a problem. I merged another PR, it caused a conflict w/ your PR.(believe it was because the other PR also introduced the communtity/builders.py ). I would have taken care of it, but it will involve removing your copyright.

Please resolve the conflict if still interested.

Thanks.

@mayn mayn removed the awaiting-reply hey! get back to me on this label Feb 4, 2019
@thor
Copy link
Author

thor commented Feb 5, 2019 via email

Uses a base class for the both of them, adding support for vsphere-iso
and vsphere-clone.
Used tests from VMware builder, that very simply verifies that things
work. Note that we do not use template, as it fails together with
troposphere included with packerlicious.

Re: troposphere, renamed keyword argument to allow for the usage of the
value "template" in a specification.
@thor
Copy link
Author

thor commented Feb 19, 2019

Conflict resolved, @mayn.

My apologies for the extended delay.

@claudekenni
Copy link

claudekenni commented Apr 19, 2019

Is this going to be merged anytime soon?
I'm interested in using this.

@thor
Copy link
Author

thor commented Apr 19, 2019

If we can get a 👍 from @mayn, I'll get this rebased, but nonetheless, if you check it out it should be usable, @claudekenni. :)

@mayn
Copy link
Owner

mayn commented Apr 22, 2019

@thor this isn’t going to get merged with the addition of your copyright header.
Let me know if it is an issue for you to remove it and I’ll close out the PR.

@claudekenni based on the outcome of the above the PR will get merged or I’ll provide you an alternate implementation.

@thor
Copy link
Author

thor commented May 10, 2019

@mayn if you must, feel free to remove the notice if you may elaborate on how it's problematic for you and the APLv2. Otherwise it seems odd and arbitrary, but beyond that, being mostly a list of properties and hardly much "code" in itself, so if there's any particular reason for it, and you may elaborate on it, then sure -- I just want to leave my tiny mark -- and get that vsphere-* support in there. 👍

@mayn mayn added this to the 1.5.0 milestone Jul 13, 2019
@mayn mayn closed this in 565df2b Jul 13, 2019
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

4 participants