Skip to content

feat(v2): AAP seed EE - #76

Open
sabre1041 wants to merge 1 commit into
redhat-cop:v2from
sabre1041:aap-seed-ee
Open

feat(v2): AAP seed EE#76
sabre1041 wants to merge 1 commit into
redhat-cop:v2from
sabre1041:aap-seed-ee

Conversation

@sabre1041

Copy link
Copy Markdown
Contributor

Description

Adds support in the aap_seed role to define an Execution Environment and associated credential

Type of Change

  • feat: A new feature
  • fix: A bug fix
  • docs: Documentation changes
  • style: Formatting, missing semi colons, etc; no code change
  • refactor: Refactoring production code
  • test: Adding missing tests, refactoring tests; no production code change
  • chore: Updating configs, etc; no production code change

@sabre1041
sabre1041 deployed to external-ci August 28, 2026 19:45 — with GitHub Actions Active
@sabre1041
sabre1041 changed the base branch from main to v2 August 28, 2026 19:46
@sabre1041
sabre1041 deployed to external-ci August 28, 2026 19:58 — with GitHub Actions Active
Signed-off-by: Andrew Block <andy.block@gmail.com>

@jeffcpullen jeffcpullen left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Great job, comments inline

aap_seed_token: "{{ aap_token | default(omit) }}"
aap_seed_validate_certs: "{{ aap_validate_certs | default(true) }}"
aap_seed_secure_logging: true
aap_seed_secure_logging: "{{ secure_logging | default(true) }}"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I saw this "global" collection variable on the last PR I reviewed. I don't believe there is any "standard" way to capture/flag/name collection level variables, but I think we should make a standard for ourselves. Reason being, if we pick a standard early they are easy to find and add to the ansible lint config to verify as a valid variable name. Since role variables are always prefixed with the role name, I would suggest the same standard be applied to the collection name. They don't need to have the whole collection name, just something unique that won't collide with other collections or roles. So my suggestion would be the prefix 'ovm_'.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This isn't a blocking suggestion as I realize it would require extensive changes to existing code. Just something to consider before we get too far into the process.

aap_seed_git_ssh_key: "{{ aap_git_ssh_key | default('') }}"
aap_seed_git_ssh_key_unlock: "{{ aap_git_ssh_key_unlock | default('') }}"

aap_seed_execution_environment_description: "{{ aap_execution_environment_description | default(omit, true) }}"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think you may be better off setting the default to an empty string rather than omitting it. Setting a variable value with the omit can get messy later if it gets called. Omit populates the variable with a bunch of garbage data.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I recommend putting the default(omit) in the actual task module and you wont have that issue

host: "{{ aap_seed_execution_environment_credential_host }}"
password: "{{ aap_seed_execution_environment_credential_password }}"
username: "{{ aap_seed_execution_environment_credential_username }}"
verify_ssl: {{ aap_seed_execution_environment_credential_verify_ssl | default(true) | bool }} No newline at end of file

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

aap_seed_execution_environment_credential_verify_ssl is neither in defaults or the argument specs

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