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

Playbook/role validation fixes (by Steampunk Spotter) #37

Closed

Conversation

gberginc
Copy link

Overall Review of Changes:

I've used Steampunk Spotter to run some checks and noticed few errors and suggestions within the playbooks/roles, regarding the used parameters in some of the tasks. Not sure if you are accepting this kind of PRs.

Enhancements:

Not sure if there is a workaround, but according to the specification of module community.windows.win_firewall should be using profiles parameter instead of profile.

This PR also fixes some other parameters that were renamed in the latest versions (albeit previous versions still work as aliases, it's good practice to keep them up to date).

How has this been tested?:
N/A

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Congrats on opening your first pull request and thank you for taking the time to help improve Ansible-Lockdown!
Please join in the conversation happening on the Discord Server as well.

@gberginc gberginc changed the title Steampunk spotter fixes Playbook/role validation fixes (by Steampunk Spotter) Aug 18, 2023
Previously used msg parameter is now replaced with the new fail_msg.

Signed-off-by: Gregor Berginc <gregor.berginc@xlab.si>
According to the documentation, the parameter to speciffy the profile
should be named profiles (plural), even if there is just one profile. This
patch replaces two occurrences.

Signed-off-by: Gregor Berginc <gregor.berginc@xlab.si>
According to the documentation, the new parameters are name
(instead of value) and type (instead of datatype). This patch
renames these occurrences.

Signed-off-by: Gregor Berginc <gregor.berginc@xlab.si>
@gberginc gberginc force-pushed the steampunk_spotter_fixes branch from 7377290 to 1527793 Compare August 22, 2023 12:51
@MrSteve81
Copy link
Contributor

Great finds I noticed your dco was bad but upon refreshing the page it looks like you have resolved that as I am typing this out.

@gberginc
Copy link
Author

Yeah, I just noticed DCO messages and signedoff my commits. Thanks for getting back to me.

We found some issues in other repos as well - is it ok to submit them as well?

@MrSteve81
Copy link
Contributor

Certainly! Thanks for the help, and feel free to jump in the discord if you want to chat about any of them.

@MrSteve81 MrSteve81 requested a review from georgenalen August 22, 2023 14:48
Copy link
Contributor

@MrSteve81 MrSteve81 left a comment

Choose a reason for hiding this comment

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

Changes Look good thanks for finding these.

@frederickw082922
Copy link
Contributor

Fixes added to #42 ! thank you @gberginc !

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.

3 participants