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

Extra variables for workflow job templates and job templates #854

Merged

Conversation

przemkalit
Copy link
Contributor

@przemkalit przemkalit commented Jul 1, 2024

What does this PR do?

Hi, I want to introduce two variables that can later be used by other filetree_create tasks. The idea behind this is to allow the export of only job templates related to certain workflows and the projects associated with these job templates. I know that having a dictionary instead of a list of IDs or names may not be necessary, but it's good to have both options to avoid querying for the ID or name.

How should this be tested?

- name: Get the Workflow
  hosts: all 
  tasks:
    - name: Retrieve the workflow
      vars:
        workflow_job_template_id: 1
      ansible.builtin.include_role: infra.controller_configuration.filetree_create

    - name: Retrieve job templates of workflow
      loop: "{{ workflow_node_jobs_dictvar | dict2items }}"
      vars:
        job_template_id: "{{ item.value }}"
      ansible.builtin.include_role: infra.controller_configuration.filetree_create
  
    - name: Retrieve the projects of job templates
      loop: "{{ projects_dictvar | dict2items }}"
      vars:
        project_id: "{{ item.value }}"
      ansible.builtin.include_role: infra.controller_configuration.filetree_create

Is there a relevant Issue open for this?

N/A

Other Relevant info, PRs, etc

This PR is related to #836.

@djdanielsson djdanielsson requested a review from ivarmu July 1, 2024 13:32
Copy link
Contributor

@ivarmu ivarmu left a comment

Choose a reason for hiding this comment

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

LGTM

@ivarmu ivarmu enabled auto-merge (squash) July 16, 2024 13:44
@adonisgarciac
Copy link
Contributor

I don't agree of defining variables in a role that will be used out of the role (they are being used in the playbook itself). Then, IMHO, that code should be part of the playbook and not part of the role.

I think the role should allow you export objects by certain IDs like was done in #836 but the role shouldn't define variables to be used out of it.

Copy link
Collaborator

@djdanielsson djdanielsson left a comment

Choose a reason for hiding this comment

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

preventing merge until discussions have taken place to decide best approach

auto-merge was automatically disabled July 18, 2024 11:49

Head branch was pushed to by a user without write access

@przemkalit
Copy link
Contributor Author

przemkalit commented Jul 18, 2024

I don't agree of defining variables in a role that will be used out of the role (they are being used in the playbook itself). Then, IMHO, that code should be part of the playbook and not part of the role.

I think the role should allow you export objects by certain IDs like was done in #836 but the role shouldn't define variables to be used out of it.

Ok, I think I got your point, please check my newest changes.

I removed the variables, and put export inside workflow and job template, and this is updated usage:

- name: Get the Workflow and related job_templates and projects
  hosts: all 
  tasks:
    - name: Retrieve the workflow
      vars:
        workflow_job_template_id: 1
      ansible.builtin.include_role: infra.controller_configuration.filetree_create

@przemkalit
Copy link
Contributor Author

I've added a new change, if variable export_related_objects is defined and true, it should export related objects, if not it won't.

@przemkalit przemkalit requested review from silvinux and a team as code owners July 26, 2024 08:11
@djdanielsson
Copy link
Collaborator

@adonisgarciac are you good with this now?

@adonisgarciac
Copy link
Contributor

@adonisgarciac are you good with this now?

I don't think so. I wrote 2 comments that were not reviewed by @przemkalit. IMO, with those changes, we're getting only one element of a list so if I'm not wrong, it does not make sense.

@przemkalit it would be great if you could look over my previous comments

@@ -12,6 +12,26 @@
order_by: 'organization,id'
no_log: "{{ controller_configuration_filetree_create_secure_logging }}"

- name: "Block for matching job templates ids with names for given workflow"
Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm not wrong, this is not going to work. This is taking only first WFJT from workflow_job_templates_lookvar which is a list. Therefore, only the nodes of first WFJT exported will be exported as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I get your point, but behind this is idea that task is exporting single WFJT with related JTs, that's why I am taking first element from the list. I could change it but I believe it would complicated because I would need check if someone is exporting only WFs and not the JTs.

@@ -13,6 +13,13 @@
order_by: 'organization,id'
no_log: "{{ controller_configuration_filetree_create_secure_logging }}"

- name: "Export project related to job_template"
Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm not wrong, this is not going to work. This is taking only first JT from job_templates_lookvar which is a list. Therefore, only the project of first JT exported will be exported as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The same goes here, idea the same, export JT with project. And that's why there is when statement which check if the id for JT is provided.

@@ -22,6 +22,7 @@ output_path: "/tmp/filetree_output"

# Maximum number of objects to return from the list. If a list view returns more an max_objects an exception will be raised
query_controller_api_max_objects: 10000
export_related_objects: true
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer to have false as default to keep previous default behavior

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved.

@przemkalit
Copy link
Contributor Author

przemkalit commented Aug 5, 2024

@adonisgarciac are you good with this now?

I don't think so. I wrote 2 comments that were not reviewed by @przemkalit. IMO, with those changes, we're getting only one element of a list so if I'm not wrong, it does not make sense.

@przemkalit it would be great if you could look over my previous comments

@adonisgarciac
Just a quick note from my side: I saw the comments in the email notifications, but I couldn't find them in the PR conversation. I thought they might have been deleted. I see the new comments and I'll check them by tomorrow.

@@ -25,6 +25,7 @@ The following variables are required for that role to work properly:
| `show_encrypted` | N/A | no | bool | Whether to remove the string '\$encrypted\$' in credentials output (not the actual credential value) |
| `omit_id` | N/A | no | bool | Whether to create output files without objects id.|
| `organization`| N/A | no | str | Default organization for all objects that have not been set in the source controller.|
| `export_related_objects` | False | no | bool | Whether to export releated objects to workflows and job templates. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Indicate that this will only apply if you're exporting a single JT or a single WFJT. For instance:

Whether to export related objects to workflows or job templates when a single JT or a single WFJT are being exported

Copy link
Contributor

Choose a reason for hiding this comment

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

solved!

@djdanielsson djdanielsson merged commit 4c08f56 into redhat-cop:devel Aug 7, 2024
3 of 5 checks passed
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.

4 participants