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

[WIP] vmware.vmware_rest: Generate action_groups #76

Closed

Conversation

mariolenz
Copy link
Contributor

@mariolenz mariolenz commented Dec 30, 2023

SUMMARY

Fixes #75

Generate action_groups for vmware.vmware_rest.

edit: Tested in ansible-collections/vmware.vmware_rest#461

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

plugins/action/generate_cloud_modules.py

ADDITIONAL INFORMATION

Same code as for the Amazon collection in generate_amazon_cloud.

This has already been requested twice by users:
ansible-collections/vmware.vmware_rest#361
ansible-collections/vmware.vmware_rest#440

@mariolenz mariolenz changed the title [DNM] vmware.vmware_rest: Generate action_groups [WIP] vmware.vmware_rest: Generate action_groups Dec 30, 2023
@mariolenz mariolenz changed the title [WIP] vmware.vmware_rest: Generate action_groups vmware.vmware_rest: Generate action_groups Dec 30, 2023
@mariolenz
Copy link
Contributor Author

@alinabuzachis @GomathiselviS What do you think about this?

meta_dir.mkdir(parents=True, exist_ok=True)

yaml_dict = {
"requires_ansible": """>=2.14.0""",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer passing this as an argument instead of embedding the value directly. However, I'm uncertain about the method and source for passing it. One immediate idea is to use it as an argument in the manifest file. We could create a separate PR to include the Ansible version in the manifest file. If you have a better solution in mind, kindly share it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, having this hard-coded is not very great. I also think that the manifest file would be a good (probably the best) place to put the ansible-core requirement.

I'll try to figure out how to do it and open a new PR. In the meantime, I'll mark this one as work in progress.

@GomathiselviS
Copy link
Collaborator

@mariolenz Thanks for implementing this feature. It will be immensely beneficial.

It looks good.

The generator currently rewrites the files if they already exist instead of managing the differences by writing or deleting them. We aimed to address this issue, but due to other pressing priorities, it hasn't been resolved yet. Do you think it's feasible to check the contents of the runtime.yml, if it exists, and selectively apply only the differences?

@mariolenz mariolenz changed the title vmware.vmware_rest: Generate action_groups [WIP] vmware.vmware_rest: Generate action_groups Jan 8, 2024
@mariolenz
Copy link
Contributor Author

The generator currently rewrites the files if they already exist instead of managing the differences by writing or deleting them. We aimed to address this issue, but due to other pressing priorities, it hasn't been resolved yet. Do you think it's feasible to check the contents of the runtime.yml, if it exists, and selectively apply only the differences?

After thinking this through, I would keep re-generating the file every time. If there effectively are no changes, git will ignore the file when you commit so there's no change, really.

But if you want to "manage the differences" someone has to code this. This code can be buggy and needs to be maintained. Code that doesn't exist can't contain any bugs and nobody has to maintain it ;-)

I don't see any benefit over rewriting the file every time. It's probably even more efficient in terms of CPU cycles and other resources.

@mariolenz
Copy link
Contributor Author

@GomathiselviS If this OK for you I'd like to keep this WIP until #77 is merged.

@GomathiselviS
Copy link
Collaborator

@mariolenz Please add unit testcase to test the generation of action_groups.

@mariolenz
Copy link
Contributor Author

mariolenz commented Jan 9, 2024

Please add unit testcase to test the generation of action_groups.

@GomathiselviS I have absolutely no idea how to do this. Any help would be very much appreciated.

Maybe I can more or less copy&paste the unit test for generating action_groups in generate_amazon_cloud but I seem unable to find it :-/

@GomathiselviS
Copy link
Collaborator

Please add unit testcase to test the generation of action_groups.

@GomathiselviS I have absolutely no idea how to do this. Any help would be very much appreciated.

Maybe I can more or less copy&paste the unit test for generating action_groups in generate_amazon_cloud but I seem unable to find it :-/

Take a look at the following file: test_generator.py. It contains testcases that might serve as a reference for adding a 'generate_action_group' testcase. If you require further information, feel free to ask.

@mariolenz
Copy link
Contributor Author

mariolenz commented Jan 10, 2024

@GomathiselviS I've closed this accidentally. Could you please reopen it again? Or should I create a new PR?

@mariolenz
Copy link
Contributor Author

@GomathiselviS I've closed this accidentally. Could you please reopen it again? Or should I create a new PR?

@GomathiselviS I've created #78

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.

Generate action_groups in runtime.yml
3 participants