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.

Sign up for GitHub

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

Jump to bottom

Add ansible-lint support #54

Open
wants to merge 63 commits into
base: main
Choose a base branch
from
Open

Add ansible-lint support #54

wants to merge 63 commits into from

Conversation

lilatomic
Copy link
Contributor

@lilatomic lilatomic commented Apr 6, 2022 •
edited
Loading

I think this is pretty decent for ansible-lint support. I dropped the generator stuff from main, I'll reintroduce it later. Currently it does something similar but much more jank by fulling in a slightly tailored **/*

Features:

  • ansible-lint support
  • support for other Ansible artifacts (Playbook&context, Role, Collection)

Tests:

  • some end2end tests
  • capture the helloansible directory to test resources

Deferred:

  • Use source generators instead of fancy globs

sureshjoshi and others added 30 commits March 30, 2022 12:24
@sureshjoshi @lilatomic
See issue sureshjoshi#4: Created a deploy goal rule
96c1298 
- Created a check goal for an ansible playbook (--syntax-check)
@sureshjoshi @lilatomic
See issue sureshjoshi#4: Added link to Readme on main page
bdcbb80
@sureshjoshi @lilatomic
See issue sureshjoshi#4: Created a barebones ansible-playbook command…
6093610 
… by re-using lint/check code
@lilatomic
first pass at ansible-lint task
8e0f41e
@lilatomic
change to ansible-lint version prior to automagic ansible detection
9b045dc 
ansible-lint will helpfully also pull in `ansible` as a module
if there is an 'ansible' in the same venv.
However, this isn't a general solution for running other binaries from the same venv.
 I would rather force using more robust features of PEX or the toolchain
so that it's easier to use this as a base for other plugins
or rules within this plugin
@lilatomic
use --venv prepend for PEX to get multiple binaries working
661241d 
This option isn't plumbed with the VenvPexRequest,
so I just use the PexRequest with additional args which do the same.
VenvPexRequest mostly just adds the `--venv` argument to the PEX creation,
so using the PexRequest in this way is mostly equivalent anyhow.

`--venv prepend` prepends the binaries to the PATH, so they're discoverable
@lilatomic
update for pants 2.11.0.dev2
76ac152
@lilatomic
expand ansible example with other playbook and tasklist
59f7559
@lilatomic
add play context as SourcesField
71d1e01
@lilatomic
add ansible context loading to linting
398415d
@lilatomic
fmt
8e2a060
@lilatomic
extract rule for turning play contexts into Digests
920e2ae
@lilatomic
extract function to get contexts from request
2d45ddd
@lilatomic
remove dead code
0031bb7 
- new way to get sources
- we no longer need to strip the top-level directory
@lilatomic
multiget for sources and ansible
3523b0f
@lilatomic
update AnsibleCheckRequest with name for pants 2.11
7f41d5b
@lilatomic
[refactor] move resolve_ansible_context
7ff0141
@lilatomic
rebuild check to use context
36720e7
@lilatomic
use mutliget in check
24b38ef
@lilatomic
get context in deployment task
0b36102
@lilatomic
lint
50d7ab7
@lilatomic
resolve Ansible context in one rule
e4ed498 
required adding wrapping type to keep graph separate
@lilatomic
better processing of arbitrary requests into contexts
bde3146
@lilatomic
use common resolver
04716fd
@lilatomic
linting doesn't need target playbooks
62f0d9d 
the playbook itself is already included as a source
@lilatomic
extract sources to own file
8fbb399
@lilatomic
pull together more sources
e8c227c
@lilatomic
clarify generation of source globs
7645913 
they're wrong, but at least they'll be easier to fix
@lilatomic
unalias source name and create sources parent class
bcf776a
@lilatomic
stub Ansible collection and role
45d90e1
@sureshjoshi
Copy link
Owner

Wow, that's a big PR :)

lilatomic added 9 commits April 6, 2022 14:48
@lilatomic
Merge branch 'main' into 4/lint
fd82c99 
rules and target_types merged too messily
@lilatomic
[merge] recapture test directory
6de0110
@lilatomic
playbooks can pull in local roles
3f06e92
@lilatomic
turn ansible-galaxy into PythonToolBase
63334f5
@lilatomic
insert galaxy requirement installation in ansible deploy
fa70106
@lilatomic
tell ansible-playbook to use installed collections
8cb2e62 
using the envvar ANSIBLE_COLLECTIONS_PATHS,
we can use the installed collections
regardless of the location of the playbook
and without relying on the local collections path.
This also means that we don't have to relocate playbooks.
Keeping playbooks in their positions in the repo
makes relative references cleaner.
For example, importing from /envs/prod/vars.yml
@lilatomic
install dependent collections in tests
0e5c96b
@lilatomic
extract galaxy fetching to rule
a650ce9
@lilatomic
install galaxy dependencies in "check" too
660b6ea
@lilatomic lilatomic marked this pull request as ready for review April 7, 2022 00:49
@lilatomic
Copy link
Contributor Author

Wow, that's a big PR :)

yeah it, uh, got away a bit. Some of it is ansible test code.

@lilatomic
Copy link
Contributor Author

Heya, I've merged HEAD into this branch, a rebase was getting messy fast.
There are some open mypy problems, most of them seems like Type Tetris related (some pants things aren't exported; I'm having trouble with some of the config settings like StrOption). LMK if you want me to work harder on those.

@lilatomic
fix more typehints
3a64c08 
the *Option types are only necessary on the Subsystem class (as classvars),
not dataclasses created from their values
@sureshjoshi
Copy link
Owner

Okay, so - in the process of reviewing. Is there a mechanism to NOT take and entire subfolder and have it owned by Ansible? Like, can the list of sources be trimmed or filtered? I'm just trying to get my head around some of the Pants ownership semantics

@sureshjoshi
Copy link
Owner

Also, I might split this PR up into a couple of them, as there's a lot of intermingled items - so it's a bit tricky to grok all the changes.

This was referenced Jun 14, 2022
Closed
Merged
@lilatomic
Copy link
Contributor Author

Okay, so - in the process of reviewing. Is there a mechanism to NOT take and entire subfolder and have it owned by Ansible? Like, can the list of sources be trimmed or filtered? I'm just trying to get my head around some of the Pants ownership semantics

do you mean on the pants side, or the ansible side? I'm pretty sure there isn't a way to affect the ansible file discovery, and all files are technically fair game for ansible. That said, there's nothing stopping us from restricting pants to only pull in a constrained subset. We could also add tailor support, so it's not so kludgy to request that people add eg role_defaults() to a role's defaults/BUILD file; that would really help constrain things, and make it more reasonable to not pull in every file ansible might.

I haven't really read up on implementing tailor or on using target generation or any of that. I'm sure they'd provide benefits over the globs I have here, but also would make this MR even bigger.

Speaking of size, LMK if there's a way you'd like this broken up and I can help with that. I'm a history packrat so I didn't see a way to disentangle this, but if you'd like me to try again but more segmented this time I can give it a shot.

@sureshjoshi
Copy link
Owner

do you mean on the pants side, or the ansible side? I'm pretty sure there isn't a way to affect the ansible file discovery, and all files are technically fair game for ansible. That said, there's nothing stopping us from restricting pants to only pull in a constrained subset. We could also add tailor support, so it's not so kludgy to request that people add eg role_defaults() to a role's defaults/BUILD file; that would really help constrain things, and make it more reasonable to not pull in every file ansible might.

I meant on the Pants side with a sources filter. I used a glob previously for simplicity, but it allows the end-user to add/remove whatever they want and constrain as needed.

I haven't really read up on implementing tailor or on using target generation or any of that. I'm sure they'd provide benefits over the globs I have here, but also would make this MR even bigger.

Can be added as a different PR. I implemented it in a couple language targets, and if we can constrain the obvious ansible files (which is hard, as it's more free-form), it's surprisingly trivial to add tailor support!

Speaking of size, LMK if there's a way you'd like this broken up and I can help with that. I'm a history packrat so I didn't see a way to disentangle this, but if you'd like me to try again but more segmented this time I can give it a shot.

I've started this split by at least moving the most logical, isolated block (the ansible-lint tool itself) into a separate PR. We can always add to it when targets get changed and all of that. If the test coverage is in place, there won't be an accidental breakage.

I was trying to add you as a reviewer to this - but couldn't last night. I gave up and went to bed instead.
#64

Incomplete PR, as I haven't finished pulling in your testing. I've also tried to line it up with some of the other formatters/linters I've added - so there is consistency (and it should also be consistent with Pants mainline linters too).

There's currently a bug somewhere I think, as I'm only seeing a subset of lint bugs, but I haven't looked into it much.

Once that PR is merged, we can pull it in, and then have a PR for each Ansible "type" (role, collection, galaxy support + testing). That way each PR is pretty constrained in scope and easy to review, and test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lilatomic @sureshjoshi