Skip to content

feature: support both upstream/downstream system_roles collections#187

Merged
djdanielsson merged 9 commits intoredhat-cop:mainfrom
Denney-tech:system_roles
May 9, 2024
Merged

feature: support both upstream/downstream system_roles collections#187
djdanielsson merged 9 commits intoredhat-cop:mainfrom
Denney-tech:system_roles

Conversation

@Denney-tech
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

Copy link
Collaborator

@jeffmcutter jeffmcutter left a comment

Choose a reason for hiding this comment

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

LGTM.

@jeffmcutter
Copy link
Collaborator

LGTM.

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

@jeffmcutter
Copy link
Collaborator

@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
Copy link
Contributor Author

Thank you. Will look at it tomorrow.

@Denney-tech Denney-tech requested a review from jeffmcutter May 6, 2024 16:20
@jeffmcutter
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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

@jeffmcutter
Copy link
Collaborator

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
Copy link
Contributor Author

@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
Copy link
Collaborator

@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
Copy link
Collaborator

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

Introduced here: which is post 24.2.3 release. ansible/ansible-lint@d910de0

Working on a fix.

@Denney-tech
Copy link
Contributor Author

Denney-tech commented May 8, 2024

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
Copy link
Collaborator

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

@djdanielsson djdanielsson merged commit e1e4dc3 into redhat-cop:main May 9, 2024
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