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

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

Merged
merged 1 commit into from
Feb 14, 2020

Conversation

jhawk28
Copy link
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
Copy link
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
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.

@azr
Copy link
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
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
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
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
Contributor

azr commented Feb 14, 2020

Superawesome; thanks @jhawk28 🙂

Copy link
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
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

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