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

Fix changed_when statement in all roles to show correct state and dis… #815

Merged
merged 5 commits into from
May 13, 2024

Conversation

keilr
Copy link
Contributor

@keilr keilr commented Apr 19, 2024

What does this PR do?

In order to display diff output and correct changed state, changed_when statement needs to be set properly.

Currently changed is just inverted with the not operator, which doesn't make sense.

How should this be tested?

We tested this in our environment. Please let me know if I can help you in adding automated tests.

Is there a relevant Issue open for this?

No

Other Relevant info, PRs, etc

In order to support diff mode I also raised the following issue and pull request:

These are related to to underlying awx.awx (= ansible.controller) modules, used in the infra.controller_configuration roles

Copy link
Collaborator

@Tompage1994 Tompage1994 left a comment

Choose a reason for hiding this comment

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

I agree this is a good change. I can see how we haven't spotted it before as well. I think we should add some check_mode testing to the tests to prove this out. If you fancy doing that, then great, otherwise I can raise an issue to fix that afterwards.

The only change I will request here though is that you add a changelog fragment (create a file under changelogs/fragments with a bugfixes item describing this change. You'll find similar examples in that directory.)

Thanks for your contribution

@ivarmu ivarmu added bug Something isn't working good first issue Good for newcomers labels Apr 30, 2024
@djdanielsson
Copy link
Collaborator

@keilr thank you for contributing back to the community, if you could just add the changeling as Tom requested we can get this merged

Copy link
Collaborator

@Tompage1994 Tompage1994 left a comment

Choose a reason for hiding this comment

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

I've added the changelog myself and this should be all good now. I'll raise a separate issue to add testing

@Tompage1994 Tompage1994 enabled auto-merge (squash) May 13, 2024 08:29
@Tompage1994 Tompage1994 merged commit 1a7113a into redhat-cop:devel May 13, 2024
12 checks passed
@keilr
Copy link
Contributor Author

keilr commented May 14, 2024

@Tompage1994 @djdanielsson thanks a lot for your feedback and for adding the changelog. Sorry for the delay, I was on vacation 🏖️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants