redhat-cop / infra.leapp

Collection of Ansible roles for automating RHEL in-place upgrades using Leapp.
MIT License
43 stars 33 forks source link

feature: support both upstream/downstream system_roles collections #187

Closed Denney-tech closed 2 months ago

Denney-tech commented 2 months ago

The crypto_policies role called by the upgrade role can dynamically reference the collection source. The changes in this PR are based on what I found in the redhat.sap_install/community.sap_install collections.

Adds infra_leapp_upgrade_system_roles_collection and defaults to fedora.linux_system_roles, and includes an arguement_spec.yml for ensuring that only that collection and redhat.rhel_system_roles can be referenced this way.

This PR should close the existing draft: #136

I consider this to be a minor change as no existing behavior should change.

However, there is a major change in order to pre-emptively fix the CI workflows.

All of the dependencies are moved from galaxy.yml to requirements.yml, and the ansible-lint workflow now installs dependencies directly from the requirements file in anticipation of this. Removing the dependencies may have an impact to customer pipelines that relied on it, but that can be easily fixed by adding the dependencies. The end result is this makes the fedora.linux_system_roles optional.

Final thoughts

The sap_install collection I referenced does not have any hard collection dependencies upstream on galaxy.ansible.com, but they do downstream on automation-hub which requires redhat.rhel_system_roles. They are in different namespaces, so I don't know how they handle that workflow or whether or not it would make any sense to do something similar for infra.leapp to build with different dependencies for galaxy/automation-hub.

Denney-tech commented 2 months ago

I think I created the new files with CRLF eol's, and that caused the CI failures (at least the linting one). Will verify later and report back.

Denney-tech commented 2 months ago

That's what I get for writing Ansible in multiple places. Converted the new files from CRLF to LF line endings.

jeffmcutter commented 2 months ago

LGTM.

I should have approved the tests first. I meant to but then I got sidetracked.

jeffmcutter commented 2 months ago

@Denney-tech, I fixed the requirements.yml file, I'll let you fix the linting. Once it passes lint I'm back to LGTM.

Denney-tech commented 2 months ago

Thank you. Will look at it tomorrow.

jeffmcutter commented 2 months ago

LGTM. I pulled this PR and tested both ways and confirmed it does use the specified collection. @djdanielsson - Do you want to double check since it does have implications on the pipeline?

djdanielsson commented 2 months ago

Idk a great way to test it without just merging it to run it through the pipeline. If it works outside of that it should work in the pipeline fine. I don't love this way but I also don't know if a better way to do it.

jeffmcutter commented 2 months ago

@Denney-tech Few more lint issues. I must have missed the run test button again.

jeffmcutter commented 2 months ago

Idk a great way to test it without just merging it to run it through the pipeline. If it works outside of that it should work in the pipeline fine. I don't love this way but I also don't know if a better way to do it.

Yeah, but I can see a time where using the rhel_system_roles could be a requirement, so I think it's good functionality to have. I vote we try it.

Denney-tech commented 2 months ago

@jeffmcutter Hopefully that's the last of the linting issues. I'm going to move my local so I'm not writing ansible in multiple places and can actually lint the code before I commit here.

jeffmcutter commented 2 months ago

@djdanielsson Seems like something with the latest version of ansible-lint? Linting of this PR passes on my system:

[jcutter@jcutter-fedora ansible-leapp{system_roles}]$ git log | head -1
Tue May 7 09:12:03 2024 -0500 6da4443 (HEAD -> system_roles) fix new-line-at-end-of-file lint issue  [Denney-tech]
[jcutter@jcutter-fedora ansible-leapp{system_roles}]$ ansible-lint

Passed: 0 failure(s), 0 warning(s) on 137 files. Profile 'production' was required, and it passed.
[jcutter@jcutter-fedora ansible-leapp{system_roles}]$ ansible-lint --version
ansible-lint 24.2.3 using ansible-core:2.16.5 ansible-compat:4.1.11 ruamel-yaml:0.18.5 ruamel-yaml-clib:0.2.8

https://github.com/redhat-cop/infra.leapp/actions/runs/8986932577/job/24715198093?pr=187#step:7:184

Says it's on a dev build?

djdanielsson commented 2 months ago

It's not failing due to ansible lint it's failing because one of the files is missing a blank line at the end of the file

Denney-tech commented 2 months ago

@djdanielsson You mean the previous run? I already fixed the missing new lines (that's my commit message in @jeffmcutter's snippet).

This last run is using a preview release of ansible-lint (24.2.4.dev10) for some reason instead of the most recent tagged release of 24.2.3.

The actual error we're seeing is with .yamllint, so the root cause might actually be in the latest yamllint since I haven't touched that file.

CRITICAL Found incompatible custom yamllint configuration (.yamllint), please either remove the file or edit it to comply with:
  - 
comments-indentation must be false  - braces.max-spaces-inside must be 1  - octal-values.forbid-implicit-octal must be true  - octal-values.forbid-explicit-octal must be true.
Denney-tech commented 2 months ago

It's ansible-lint: https://ansible.readthedocs.io/projects/lint/rules/yaml/#yamllint-configuration

Introduced here: which is post 24.2.3 release. https://github.com/ansible/ansible-lint/commit/d910de099a3dbb18670ae07772a9a556083f9954

Working on a fix.

Denney-tech commented 2 months ago

Added missing requirements, but unable to upgrade ansible-lint to test locally as my work's internal pypi mirror is stale. Time to put in a support ticket...

jeffmcutter commented 2 months ago

At last we have passed lint. :-) @djdanielsson Can you do the honors?