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

Use MIRSG molecule-test #5

Merged
merged 12 commits into from
Nov 30, 2023
Merged

Use MIRSG molecule-test #5

merged 12 commits into from
Nov 30, 2023

Conversation

p-j-smith
Copy link
Contributor

@p-j-smith p-j-smith commented Nov 28, 2023

Reduce duplication in workflows by:

  • using our molecule-test GitHub Action
  • adding a reusable workflow for running the action
  • use this reusable workflow for running tests on the provision and firewalld roles (more workflows can be added in a separate pr)
  • run the tests for a given role when either the role itself changes or the workflow changes

Note, this depends on UCL-MIRSG/.github#80 being merged first.
And linting is broken will fix in a different pr

@p-j-smith p-j-smith requested a review from a team November 28, 2023 13:20
Copy link
Member

@paddyroddy paddyroddy left a comment

Choose a reason for hiding this comment

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

Seems a bit odd to use a reusable workflow to call a composite workflow? Perfectly valid, but is it needed?

@@ -11,7 +11,7 @@ jobs:
linting:
runs-on: ubuntu-latest
steps:
- uses: UCL-MIRSG/.github/actions/linting@v0.36.0
- uses: UCL-MIRSG/.github/actions/linting@v0.38.0
Copy link
Member

Choose a reason for hiding this comment

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

One day Renovate will come back 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can only dream

Copy link
Contributor

Choose a reason for hiding this comment

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

Dreams do come true - I just reenabled it...

I put it on the shared calendar to reenable, but I still forgot...

Copy link
Member

Choose a reason for hiding this comment

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

I was scared to re-enable it haha

scenario: ${{ matrix.scenario }}
checkout_path: ansible_collections/mirsg/infrastructure
tests_path: ansible_collections/mirsg/infrastructure/tests

Copy link
Member

Choose a reason for hiding this comment

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

missing new line at the end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think new line was there but it wasn't empty 😬

Copy link
Member

Choose a reason for hiding this comment

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

Ah, just saw the dreaded 🚫

fail-fast: true
matrix:
scenario:
- centos7
Copy link
Member

Choose a reason for hiding this comment

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

Only centos7?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah for now, I'll add Rocky 9 in a separate pr

on:
workflow_call:
inputs:
run_tags:
Copy link
Member

Choose a reason for hiding this comment

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

Generally people use run-tags rather than run_tags

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've renamed it now

@p-j-smith
Copy link
Contributor Author

Seems a bit odd to use a reusable workflow to call a composite workflow? Perfectly valid, but is it needed?

Yeah it is a little convoluted. The molecule-test action that's in our .github repo can be used both for testing this collection as well as for testing standalone roles. When testing the roles within this collection there's a bit of boilerplate that's needed (and that we'd use for each role we want to test). So I put this boilerplate into a reusable workflow. We could have a workflow molecule-test-collection but that would duplicate a lot of what's in the existing molecule-test action. Roundabout way of saying it's not really necessary, just seemed like a sensible way of doing things. Happy to change though!

@paddyroddy
Copy link
Member

Roundabout way of saying it's not really necessary, just seemed like a sensible way of doing things. Happy to change though!

Happy with whatever you decide. If it's a duplication concern, I assume one composite action can depend on another (maybe)?

@p-j-smith p-j-smith mentioned this pull request Nov 28, 2023
@p-j-smith
Copy link
Contributor Author

Happy with whatever you decide. If it's a duplication concern, I assume one composite action can depend on another (maybe)?

Yeah we could have another action molecule-test-collection that uses the existing molecule-test action, and have each workflow use that molecule-test-collection. That new action would look like this:

---
name: Molecule Test Collection
description: Run `molecule test` on this Ansible Collection
inputs:
scenario:
description: Name of the scenario to run the tests on
required: false
default: default
runs:
using: composite
steps:
- name: Run `molecule test`
uses: UCL-MIRSG/.github/actions/[email protected]
with:
scenario: ${{ inputs.scenario }}
checkout_path: ansible_collections/mirsg/infrastructure
tests_path: ansible_collections/mirsg/infrastructure/tests

And using it in a workflow:

name: Test firewalld
on:
pull_request:
paths:
- "roles/firewalld/**"
- ".github/actions/molecule-test-collection/action.yml"
- ".github/workflows/molecule-firewalld-with-action.yml"
jobs:
molecule:
runs-on: ubuntu-latest
strategy:
fail-fast: true
matrix:
scenario:
- centos7
env:
MOLECULE_RUN_TAGS: firewalld
PY_COLORS: 1
ANSIBLE_FORCE_COLOR: 1
steps:
- name: Run `molecule test`
uses: ./.github/actions/molecule-test-collection
with:
scenario: ${{ matrix.scenario }}

Compared to the current workflow for the same role:

name: Test firewalld
on:
pull_request:
paths:
- "roles/firewalld/**"
- ".github/workflows/molecule.yml"
- ".github/workflows/molecule-firewalld.yml"
jobs:
molecule-firewalld:
uses: ./.github/workflows/molecule.yml
with:
run-tags: firewalld

So using a reusable workflow allows us to avoid some boilerplate that can't be avoided when using a composite action

@p-j-smith p-j-smith self-assigned this Nov 28, 2023
Comment on lines 6 to 8
required: false
type: string
default: all
Copy link
Member

Choose a reason for hiding this comment

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

required isn't required if a default is set

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well spotted, I've removed it

@p-j-smith p-j-smith merged commit 1b40cb3 into main Nov 30, 2023
2 of 3 checks passed
@p-j-smith p-j-smith deleted the maint/mirsg-molecule-action branch November 30, 2023 11:07
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