Skip to content

Conversation

@gionn
Copy link
Member

@gionn gionn commented Apr 29, 2025

  • Merge dynamic-facts.yml into facts.yml early in the main playbook

OPSEXP-3231

@gionn gionn self-assigned this Apr 29, 2025
@gionn gionn force-pushed the OPSEXP-3231-hxi-role branch from 50c76c3 to 37b82da Compare May 6, 2025 08:06
@gionn gionn force-pushed the OPSEXP-3231-hxi-role branch from 0c64fd8 to f9afe52 Compare May 6, 2025 15:24
@gionn gionn force-pushed the OPSEXP-3231-hxi-role branch from d805a76 to 574689e Compare May 7, 2025 15:46
@gionn gionn requested a review from pmacius May 8, 2025 10:57
@gionn gionn force-pushed the OPSEXP-3231-hxi-role branch from 5fd9dd8 to 514c6b8 Compare May 12, 2025 14:47
@gionn gionn added the ec2-test Triggers ec2 integrations tests label May 12, 2025
@gionn gionn requested review from alxgomz and pmacius May 12, 2025 15:16
pmacius
pmacius previously approved these changes May 12, 2025
Copy link
Contributor

@pmacius pmacius left a comment

Choose a reason for hiding this comment

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

Mention something about integrating dynamic-facts.yml into facts.yml in merge commit

Copy link
Contributor

@alxgomz alxgomz left a comment

Choose a reason for hiding this comment

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

LGTM overall, just a few comments

Comment on lines 771 to 773
- name: Include HxInsight vars
ansible.builtin.include_vars:
file: ../vars/hxi.yml
Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably safe to use a var_files at the play level instead or plain set_facts tasks

Copy link
Member Author

Choose a reason for hiding this comment

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

here could be possible as the conditional in the task is not necessary, but maybe I prefer staying with the task to be consistent with the repository play

repository_truststore_type: "{{ acs_play_default_truststore_type }}"
- name: Include HxInsight vars
ansible.builtin.include_vars:
file: ../vars/hxi.yml
Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably safe to use a var_files at the play level instead or plain set_facts tasks

Copy link
Member Author

Choose a reason for hiding this comment

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

there is a conditional, which can be applied only to a task.

not sure ho set_facts can help here, hxi.yml contains configuration for hxinsight the user is meant to provide manually

just realized that I didn't write a single docs about this

@gionn gionn marked this pull request as ready for review May 13, 2025 08:21
@gionn gionn requested review from alxgomz and pmacius May 13, 2025 08:22
@gionn gionn removed the ec2-test Triggers ec2 integrations tests label May 13, 2025
pmacius
pmacius previously approved these changes May 13, 2025
@gionn gionn merged commit 04cbaac into master May 14, 2025
66 checks passed
@gionn gionn deleted the OPSEXP-3231-hxi-role branch May 14, 2025 11:00
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.

4 participants