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 project_update role/collection install #14065

Merged
merged 4 commits into from
Feb 21, 2024
Merged

Conversation

bcoca
Copy link
Member

@bcoca bcoca commented May 31, 2023

  • avoid looping
  • avoid using multiple files, only one should be provided and processed per type
  • use first_found and variables to locate existing file
  • skip if no file exists

-->

ISSUE TYPE
  • Bug, Docs Fix or other nominal change
COMPONENT NAME
  • Other
AWX VERSION
all
ADDITIONAL INFORMATION

@shanemcd
Copy link
Member

It looks like project updates are failing with this change but the error handling in the smoke test isn't great

Exception: project_update 2 (failed) encountered an error (rc=None), please see task stdout for details.

It should be relatively trivial to spin up the dev env and see what's going on.

@AlanCoding
Copy link
Member

In the current system, it installs all files. I'm not sure if it would be preferable to install just 1, when all are present. We need some better establishment of the requirements here.

@bcoca
Copy link
Member Author

bcoca commented May 31, 2023

Yes, currently all files are used, i thought that was a bug, it seemed preferable to have a single file with all the info vs letting the user 'partition' the info across 4 files.

I have checked the few projects i have access to and none seems to use more than one file .. but i have no idea how representative that is.

@AlanCoding
Copy link
Member

I agree with this, and want to ask @ffirg to look it over because it is a behavior change.

IMO, it's a sensible change and we should merge it along with #14081, and then use it as an opportunity to better document the policy for which requirements files get installed.

Changes:

  • (this PR) if more than 1 requirements file is present under a folder (like roles/requirements.yml and roles/requirements.yaml) then only the 1st will be installed according to our precedence rules
  • (other PR) if a root-level ./requirements.y[a]ml file is present then it will be installed with the ansible-galaxy install command

@AlanCoding
Copy link
Member

Another change to the project update playbook has merged which is where the conflicts come from. I'm fine to make this change (although needs a release note) after a rebase. If you don't want to do the rebase, I can pick it up.

@bcoca
Copy link
Member Author

bcoca commented Jul 6, 2023

i used the online editor, so i dont have a branch to rebase

@AlanCoding
Copy link
Member

I rebased and pushed to your fork.

Your change was only previously applied to 2 tasks, but @john-westcott-iv's overlapping change turned those 2 tasks into 3 tasks, so your pattern was applied to that new "unified" requirements.yml task.

Also made small style changes to get all 3 tasks more consistent.

I think this is ready for review and testing.

Copy link
Member Author

@bcoca bcoca left a comment

Choose a reason for hiding this comment

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

lgtm

@AlanCoding
Copy link
Member

Getting some more data now:

The conditional check 'req_file is file' failed.
The error was:
  An unhandled exception occurred while templating '{{ req_candidates | first_found }}'.
  Error was a <class 'ansible.errors.AnsibleError'>, original message:
    template error while templating string: Could not load "first_found": 'first_found'.
    String: {{ req_candidates | first_found }}.
      Could not load "first_found": 'first_found'

The error appears to be in '/runner/project/project_update.yml': line 217, column 11, but may
be elsewhere in the file depending on the exact syntax problem.

The offending line appears to be:

    - block:
        - name: Fetch galaxy roles from roles/requirements.(yml/yaml)
          ^ here

@AlanCoding
Copy link
Member

https://docs.ansible.com/ansible/latest/collections/ansible/builtin/first_found_lookup.html

does

          vars:
            req_file: "{{ req_candidates | first_found }}"

need to be

          vars:
            req_file: "{{ lookup('ansible.builtin.first_found', req_candidates) }}"

bcoca and others added 4 commits February 21, 2024 14:16
  - avoid looping
  - avoid using multiple files, only one should be provided and processed per type
  - use first_found and variables to locate existing file
  - skip if no file exists
@dmzoneill dmzoneill enabled auto-merge (rebase) February 21, 2024 14:23
@dmzoneill dmzoneill merged commit 14454cc into ansible:devel Feb 21, 2024
21 checks passed
@bcoca bcoca deleted the patch-2 branch March 15, 2024 01:19
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