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

refactor: consolidate player driver #231

Merged
merged 1 commit into from
Oct 7, 2024

Conversation

tenthirtyam
Copy link
Collaborator

@tenthirtyam tenthirtyam commented Jul 18, 2024

Description

Consolidates Player5Driver and Player6Driver to PlayerDriver within driver_player.go.

Testing

  • Basic
  • End-to-End
packer-plugin-vmware on  refactor/consolidate-player-drivergo fmt ./...

packer-plugin-vmware on  refactor/consolidate-player-drivermake generate                                            
2024/07/18 18:07:15 Copying "docs" to ".docs/"
2024/07/18 18:07:15 Replacing @include '...' calls in .docs/
Compiling MDX docs in '.docs' to Markdown in '.web-docs'...

packer-plugin-vmware on  refactor/consolidate-player-drivermake build   

packer-plugin-vmware on  refactor/consolidate-player-drivermake test    
?       github.com/hashicorp/packer-plugin-vmware       [no test files]
?       github.com/hashicorp/packer-plugin-vmware/version       [no test files]
ok      github.com/hashicorp/packer-plugin-vmware/builder/vmware/common 6.704s
ok      github.com/hashicorp/packer-plugin-vmware/builder/vmware/iso    2.066s
ok      github.com/hashicorp/packer-plugin-vmware/builder/vmware/vmx    2.544s

packer-plugin-vmware on  refactor/consolidate-player-driver make dev     
packer plugins install --path packer-plugin-vmware "github.com/hashicorp/vmware"
Successfully installed plugin github.com/hashicorp/vmware from /Users/ryan/Library/Mobile Documents/com~apple~CloudDocs/Code/Personal/packer-plugin-vmware/packer-plugin-vmware to /Users/ryan/.packer.d/plugins/github.com/hashicorp/vmware/packer-plugin-vmware_v1.0.12-dev_x5.0_darwin_arm64

@tenthirtyam tenthirtyam added the hypervisor/player VMware Player label Jul 18, 2024
@tenthirtyam tenthirtyam self-assigned this Jul 18, 2024
@tenthirtyam tenthirtyam added this to the v1.1.1 milestone Jul 19, 2024
@tenthirtyam tenthirtyam force-pushed the refactor/consolidate-player-driver branch 2 times, most recently from 5a828c6 to 82753c2 Compare August 9, 2024 18:21
@tenthirtyam tenthirtyam marked this pull request as ready for review August 10, 2024 03:41
@tenthirtyam tenthirtyam requested a review from a team as a code owner August 10, 2024 03:41
@tenthirtyam tenthirtyam marked this pull request as draft August 26, 2024 15:58
@tenthirtyam tenthirtyam force-pushed the refactor/consolidate-player-driver branch 5 times, most recently from 8455cd4 to 56e5c45 Compare August 28, 2024 03:48
@tenthirtyam tenthirtyam force-pushed the refactor/consolidate-player-driver branch 5 times, most recently from bd0479a to 537d592 Compare September 10, 2024 02:01
@tenthirtyam tenthirtyam marked this pull request as ready for review September 10, 2024 02:28
@tenthirtyam
Copy link
Collaborator Author

@lbajolet-hashicorp - this one is ready for initial review for the driver consolidation.

E2E testing is still pending but I will be testing it this week on both Windows (11) and Linux (Ubuntu 22.04) with Workstation Player 17.6.

Ryan

Copy link
Contributor

@lbajolet-hashicorp lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

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

Left a couple nits, but overall this LGTM, thanks @tenthirtyam!

Pre-approving now, we can merge once you've had a chance to address my comments

builder/vmware/common/driver_player.go Show resolved Hide resolved
builder/vmware/common/driver_player.go Outdated Show resolved Hide resolved
Consolidates `Player5Driver` and `Player6Driver` to `PlayerDriver` within `driver_player.go`.

Signed-off-by: Ryan Johnson <ryan@tenthirtyam.org>
@tenthirtyam tenthirtyam force-pushed the refactor/consolidate-player-driver branch from 7229c55 to ca282ef Compare October 7, 2024 16:54
Copy link
Contributor

@lbajolet-hashicorp lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the rerolls @tenthirtyam

Merging now!

@lbajolet-hashicorp lbajolet-hashicorp merged commit 3b080bf into main Oct 7, 2024
14 checks passed
@lbajolet-hashicorp lbajolet-hashicorp deleted the refactor/consolidate-player-driver branch October 7, 2024 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants