Skip to content

add ability to define multiple NICs for vsphere-iso#8739

Merged
sylviamoss merged 1 commit intohashicorp:masterfrom
jhawk28:vsphere_multiple_nics
Feb 14, 2020
Merged

add ability to define multiple NICs for vsphere-iso#8739
sylviamoss merged 1 commit intohashicorp:masterfrom
jhawk28:vsphere_multiple_nics

Conversation

@jhawk28
Copy link
Copy Markdown
Contributor

@jhawk28 jhawk28 commented Feb 14, 2020

Added the ability to define multiple network adapters and their respective parameters for the vsphere-iso builder. Network and network_card parameters are still there for backwards compatibility (will be used as the first network adapter if defined). Added the ability to manually define a mac address for the NIC and also enable/disable the directpath i/o.

Closes #8719
Closes #8618

@jhawk28 jhawk28 requested a review from a team as a code owner February 14, 2020 03:51
@jhawk28 jhawk28 force-pushed the vsphere_multiple_nics branch from 7ede1ac to b0bb525 Compare February 14, 2020 14:46
Copy link
Copy Markdown
Contributor

@azr azr left a comment

Choose a reason for hiding this comment

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

Super nice one, LGTM, thanks for the contrib.

This is a config breaking change and I think we will need to wait for a minor release (1.6.0) to be planned to merge this.

@jhawk28
Copy link
Copy Markdown
Contributor Author

jhawk28 commented Feb 14, 2020

It still accepts the old configs. I did not remove any parameters (network+networkCard). network_adapters was added. Should not be a breaking change.

@jhawk28 jhawk28 requested a review from sylviamoss February 14, 2020 16:01
@azr
Copy link
Copy Markdown
Contributor

azr commented Feb 14, 2020

Network,NetworkCard are nested into an array, so yes they are still there but the users who already have a conf with an unnested networkCard and or network will need to fix their config file. BTW I think we also need to add a fixer to this, you can find examples in the fix/ folder.

@jhawk28
Copy link
Copy Markdown
Contributor Author

jhawk28 commented Feb 14, 2020

The original network and network_card fields are still there (step_create:45-48). You can see how I generate a NIC struct with them on step_connect:102. If we want to go the fix route, then I can remove the fields and add a "fix".

@azr
Copy link
Copy Markdown
Contributor

azr commented Feb 14, 2020

Oh right, I'm sorry I though they were removed because of this change: https://github.com/hashicorp/packer/pull/8739/files#diff-57d6257e1da6868d360bd78403640dd9L59-L60

@azr azr modified the milestones: 1.6.0, 1.5.3 Feb 14, 2020
@jhawk28
Copy link
Copy Markdown
Contributor Author

jhawk28 commented Feb 14, 2020

No problem. The vpshere-iso configuration vs driver data split is a little weird. I'll do a second pull request for removing the old fields and adding the "fix" to clean things up for 1.6.0.

@azr
Copy link
Copy Markdown
Contributor

azr commented Feb 14, 2020

Superawesome; thanks @jhawk28 🙂

Copy link
Copy Markdown
Contributor

@sylviamoss sylviamoss left a comment

Choose a reason for hiding this comment

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

It looks good to me! Let's just get another approval and we can merge this one 👍

@sylviamoss
Copy link
Copy Markdown
Contributor

Ops, already have two approvals 😄 Nice!

@sylviamoss sylviamoss merged commit 7f0de5f into hashicorp:master Feb 14, 2020
@jhawk28 jhawk28 deleted the vsphere_multiple_nics branch March 12, 2020 16:42
@ghost
Copy link
Copy Markdown

ghost commented Mar 16, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Mar 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

vsphere-iso multiple NIC support DirectPath I/O Setting support

3 participants