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 detect if running on VMware system #193

Merged
merged 2 commits into from
Mar 1, 2023
Merged

Conversation

jamie-suse
Copy link
Contributor

No description provided.

@jamie-suse
Copy link
Contributor Author

While making this PR, I found I was writing a lot of repeated code. Thoughts on a possible refactor?

@jamie-suse jamie-suse marked this pull request as ready for review February 28, 2023 13:11
Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

If we want to reduce the amount of times the systemd-detect-virt command is run, we could maybe create a new function like systemdDetectVirt which returns the command output value and then:

  1. have some map like {kvm: true, vmware: true} or similar. Once the command is executed you could look up in the map, and if there is a value you know which is the detected one. In the other hand, you could return nil.

  2. or simply continue having identifyKVM and identifyVMware functions, but using the command output as parameter and simply do the comparison there.

Take some time to think about other solutions as well.
Anyway, the current code should work, and even with high level of duplicated code, I find it easy to navigate

internal/core/cloud/metadata.go Outdated Show resolved Hide resolved
Copy link
Member

@CDimonaco CDimonaco left a comment

Choose a reason for hiding this comment

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

Agree with @arbulu89 the code duplication and the command execution is really duplicated, the code could be a lot improved even in term of readability.

BUT, this is not the scope of this pr, the code works so we can think about later :D

Copy link
Contributor

@rtorrero rtorrero left a comment

Choose a reason for hiding this comment

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

Code looks good, green light from my side and we can later tackle how to reduce the duplication of code.

@jamie-suse jamie-suse merged commit 2fa5382 into main Mar 1, 2023
@jamie-suse jamie-suse deleted the identify-vmware branch March 1, 2023 11:12
@arbulu89 arbulu89 added the enhancement New feature or request label Apr 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

4 participants