-
Notifications
You must be signed in to change notification settings - Fork 341
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
Add github action for sanity and unit tests #1393
Conversation
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
Build failed. ✔️ ansible-galaxy-importer SUCCESS in 4m 32s |
I would suggest using pre-commit github action |
It's possible to run many of these tests locally (we should probably do a better job of documenting this), however they usually take a couple of minutes to run, so running them as part of the upload process would probably be frustrating. Additionally, some folks struggle to understand the errors. It's much easier to collaborate with them once it's uploaded and the errors are publically available, So I'd recommend not blocking commits or the submission of pushes on broken tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I like the direction this is taking. Some comments inline.
.github/workflows/tests.yml
Outdated
|
||
jobs: | ||
ansible-lint: | ||
uses: ansible-network/github_actions/.github/workflows/ansible-lint.yml@main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uses: ansible-network/github_actions/.github/workflows/ansible-lint.yml@main | |
uses: ansible-network/github_actions/.github/workflows/ansible-lint.yml@main |
Can we move these from ansible-network out into ansible-community somewhere please? There's a lot of random stuff living under ansible-network and it's really confusing if you're not familiar with the history.
Having them live in ansible-network gives the impression that either they're specifically intended only for the network modules. Or that the ansible-network group are the sole owners of these actions. IMHO they would be better "owned" by the community, and people outside of the network folks may be more inclined to contribute to them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @Qalthos .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at actions/runner#2347 I have a concern that we can't do "optional" tests. There doesn't seem to be a clean way to allow "devel" tests to fail while still blocking merges if the tests fail. GitHub also don't seem interested in offering such a feature :/ Had a quick poke about with "experimental", but I don't see a clean way to do conditional experimental |
It is not a must to activate it on local repos. Just invoke it during the CI workflow. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Build failed. ✔️ ansible-galaxy-importer SUCCESS in 4m 52s |
Cleanup and move tasks (vs playbooks) in inventory tests SUMMARY Inventory plugin tests currently mix "tasks" content in with playbooks, this triggers ansible-lint warnings. ISSUE TYPE Tests Pull Request COMPONENT NAME ec2_vpc_net plugins/inventory/aws_ec2.py plugins/inventory/aws_rds.py ADDITIONAL INFORMATION Spotted by work on #1393 Reviewed-by: Alina Buzachis
Build failed. ✔️ ansible-galaxy-importer SUCCESS in 4m 07s |
Build failed. ✔️ ansible-galaxy-importer SUCCESS in 4m 36s |
|
||
jobs: | ||
linters: | ||
uses: abikouo/github_actions/.github/workflows/tox-linters.yml@tox_linters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My preference would be to hold off on merging until we get these changes merged into the ansible-network repo, as I'd like to avoid calling out to personal repos. I won't block on this if people feel strongly that this is worth merging now, but ultimately, I want our CI burden to be shared more broadly than it has been in the past. Otherwise, I think everything looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I thought we were talking about merging things into the repo in the ansible-network org, without waiting for that repo to be migrated to the ansible-community org. Is that not the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are calls to @abikouo's repos which we will update in a separate PR, post the migration of github-actions repo.
@jillr For what it's worth we're already pulling from @abikouo's fork in "update collection variables":
I think |
all these calls to personal repo will be changed once we agree where this should live |
Minor Sanity test fixes SUMMARY Steal sanity fixes from #1393 ISSUE TYPE Feature Pull Request COMPONENT NAME plugins/modules/backup_tag_info.py plugins/modules/backup_vault.py plugins/modules/route53_info.py test-requirements.txt ADDITIONAL INFORMATION Reviewed-by: Alina Buzachis
Co-authored-by: Kate Case <[email protected]>
Co-authored-by: Kate Case <[email protected]>
The sanity fixes have been merged as #1462 and I've stripped this back to just the GitHub Action related pieces |
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 3m 47s |
Thank you |
Build succeeded (gate pipeline). ✔️ ansible-galaxy-importer SUCCESS in 5m 34s |
Pull request merge failed: Resource not accessible by integration, You may need to manually rebase your PR and retry. |
SUMMARY
Add github action workflow.
ISSUE TYPE
COMPONENT NAME
ADDITIONAL INFORMATION