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

Use ansible_facts to reference facts #893

Merged
merged 1 commit into from
Jul 24, 2023
Merged

Use ansible_facts to reference facts #893

merged 1 commit into from
Jul 24, 2023

Conversation

GregWhiteyBialas
Copy link

By default, Ansible injects a variable for every fact, prefixed with
ansible_. This can result in a large number of variables for each host,
which at scale can incur a performance penalty. Ansible provides a
configuration option [0] that can be set to False to prevent this
injection of facts. In this case, facts should be referenced via
ansible_facts..

This change updates all references to Ansible facts from using
individual fact variables to using the items in the
ansible_facts dictionary. This allows users to disable fact variable
injection in their Ansible configuration, which may provide some
performance improvement.

[0] https://docs.ansible.com/ansible/latest/reference_appendices/config.html#inject-facts-as-vars

@teddytpc1
Copy link
Member

Hi @GregWhiteyBialas.
The PR should contain only this commit. It seems that you have master changes in your 4.4 branch.

@GregWhiteyBialas
Copy link
Author

Sorry for this. Fixed.

vcerenu
vcerenu previously approved these changes Mar 21, 2023
Copy link
Member

@jnasselle jnasselle left a comment

Choose a reason for hiding this comment

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

Hi @GregWhiteyBialas,

Gj here! thanks for contributing to Wazuh!

They're still a file that need to be modified in order to be 100% compatible

https://github.com/wazuh/wazuh-ansible/blob/4.4/.github/playbooks/single-wazuh.yml

@GregWhiteyBialas
Copy link
Author

Oh, sorry for this! I didn't expect anything in there. God lesson for the future. Fixed!

@GregWhiteyBialas GregWhiteyBialas requested review from jnasselle and removed request for teddytpc1 March 22, 2023 12:12
Copy link
Member

@jnasselle jnasselle left a comment

Choose a reason for hiding this comment

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

Hi @GregWhiteyBialas ,

Sorry about requesting changes again, but after a deeper search, I found other variables that should be changed

https://github.com/wazuh/wazuh-ansible/blob/4.4/roles/wazuh/ansible-wazuh-agent/defaults/main.yml#L73

  • ansible_hostname to ansible_facts.hostname
  • ansible_default_ipv4.address to ansible_facts.default_ipv4.address

Thanks again!

@GregWhiteyBialas
Copy link
Author

Hi @jnasselle
It is I who should say sorry :)
I have made requested changes, I hope it s all good now.

@GregWhiteyBialas
Copy link
Author

I have squashed commits in this PR to, avoid clutter in history.

Copy link
Member

@jnasselle jnasselle left a comment

Choose a reason for hiding this comment

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

LGTM

@teddytpc1
Copy link
Member

Hi @GregWhiteyBialas.
Thanks for the changes. This PR will be merged after the 4.4.0 release.

3 similar comments
@teddytpc1
Copy link
Member

Hi @GregWhiteyBialas.
Thanks for the changes. This PR will be merged after the 4.4.0 release.

@teddytpc1
Copy link
Member

Hi @GregWhiteyBialas.
Thanks for the changes. This PR will be merged after the 4.4.0 release.

@teddytpc1
Copy link
Member

Hi @GregWhiteyBialas.
Thanks for the changes. This PR will be merged after the 4.4.0 release.

@GregWhiteyBialas
Copy link
Author

@teddytpc1 I have signed commit with gpg, can we merge it ?

@GregWhiteyBialas
Copy link
Author

Hi @teddytpc1 Is there something that I can do to push this forward ?

@teddytpc1 teddytpc1 merged commit 86ad00d into wazuh:4.4 Jul 24, 2023
@teddytpc1 teddytpc1 changed the title use ansible_facts to reference facts Use ansible_facts to reference facts Jul 24, 2023
@teddytpc1
Copy link
Member

Changes have been tested with the Demo deployment. It worked fine.

@jonhattan
Copy link

This changes where merged on 4.4 branch and they're not present in master. Can you please move this changes into master?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants