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 vcsa_settings module #19

Merged
merged 3 commits into from
May 28, 2024

Conversation

machacekondra
Copy link
Collaborator

System module is used to Configure system settings of VMware appliance.

@machacekondra machacekondra requested a review from bardielle May 17, 2024 11:44
@machacekondra machacekondra force-pushed the system_module branch 5 times, most recently from 487a941 to 1eded2d Compare May 17, 2024 12:38
@mariolenz
Copy link
Collaborator

@machacekondra @bardielle I didn't have a closer look the the code, but could I suggest a different name for the module? system is pretty generic. It looks like this module configures the vCenter / vCSA, not any guest or (ESXi) host or any other system.

If I'm correct, I suggest to call the module vcenter_system or vcsa_system or similar. Additionally, I would update the description. At the moment it says Configure system settings of VMware appliance but there are a lot of VMware appliances. If the module is about the vCenter appliance, as I think it is, it should state this clearly.

Sorry if I understand the module wrong. I didn't have the time for a closer look.

@bardielle
Copy link
Collaborator

@mariolenz I agree with you it will be more clear.

@mariolenz
Copy link
Collaborator

vcsa_(system_)settings or vcsa_(system_)config might also be good names for the module. I'm not sure what would be the best name, but system alone is a little bit misleading IMHO.

@machacekondra
Copy link
Collaborator Author

@mariolenz What about call it appliance_settings or something? I don't know much of the VMware terminology, so would be great to hear your advice. The module basically configure anything which is exposed by vcenter on port 5480. So I am open to any of your suggestions.

@mariolenz
Copy link
Collaborator

@machacekondra I would say that virtual appliance (often delivered in form of an OVF) is still too generic. There are both different virtual appliances from VMware (like Aria Operations, if it's still called that) and others like Cisco, HPE, Dell, NetApp... ESXi itself has an appliance-like quality, too. Although it's not a virtual appliance, usually.

So maybe call it vcsa_settings? vCSA is the official name and stands for vCenter Server Appliance with vCenter Server being the original software to manage ESXi hosts and had to be installed on a Windows Server. The vCSA is, well, the SW delivered in form of a virtual appliance. That is, including an OS. Photon OS, I think 🤔

BTW I think port 5480 often is called the VAMI (Virtual Appliance Management Interface, or something very similar).

@ihumster What do you think? It's easy to fix a description in the documentation but hard to rename a module. So we should try and have a good name from the beginning.

@ihumster
Copy link

Yep, I think vcsa (how abbreviation denoting vCenter Server Appliance) a good name to designate a module that configures appliance.

Modules starts with vcenter_ will configure something inside vCenter Server.

@machacekondra
Copy link
Collaborator Author

machacekondra commented May 20, 2024

Renamed to vcsa_settings, thanks for feedback!

@machacekondra machacekondra force-pushed the system_module branch 2 times, most recently from 9b9ed73 to f39a4ca Compare May 21, 2024 08:59
vcsa_settings module is used to Configure settings of VMware
appliance.

Signed-off-by: Ondra Machacek <[email protected]>
@machacekondra machacekondra changed the title Add system module Add vcsa_settings module May 23, 2024
@bardielle
Copy link
Collaborator

@mariolenz can we merge this PR? Or do you have any other comments?

@mariolenz
Copy link
Collaborator

@bardielle Hmm... I think there are some possible improvements. I'll add some suggestions.

Copy link
Collaborator

@mariolenz mariolenz left a comment

Choose a reason for hiding this comment

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

I suggest to use Semantic markup in the documentation.

plugins/modules/vcsa_settings.py Outdated Show resolved Hide resolved
plugins/modules/vcsa_settings.py Outdated Show resolved Hide resolved
@machacekondra
Copy link
Collaborator Author

I suggest to use Semantic markup in the documentation.

Thank you, Mario.

Copy link
Collaborator

@mariolenz mariolenz left a comment

Choose a reason for hiding this comment

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

LGTM

@bardielle bardielle merged commit f62f9b8 into ansible-collections:main May 28, 2024
9 checks passed
@Andersson007
Copy link
Contributor

module: vcsa_settings
short_description: Configure vCenter Server Appliance settings
version_added: 2.0.0

I can see it's been already released in 1.2.0. FYI

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.

5 participants