Skip to content

Molecule#76

Merged
ralphlange merged 10 commits into
epics-training:mainfrom
simon-ess:molecule
Jun 18, 2026
Merged

Molecule#76
ralphlange merged 10 commits into
epics-training:mainfrom
simon-ess:molecule

Conversation

@simon-ess

@simon-ess simon-ess commented Feb 26, 2026

Copy link
Copy Markdown
Contributor

Starting steps to add molecule tests for the roles. Includes a github job to run the tests.

@simon-ess simon-ess force-pushed the molecule branch 6 times, most recently from 1ed0af0 to 67cfec6 Compare February 26, 2026 17:21
@ralphlange

ralphlange commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

@simon-ess: Can you rebase this? I mainly want to trigger a new CI run, since the CI has changed drastically since you created the PR.

With #88 requesting many changes only tested on one flavor, I would love to know how and be able to add tests.

@simon-ess

simon-ess commented Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

Ah, thanks for reminding me. I will look at this tomorrow.

@mdavidsaver mdavidsaver mentioned this pull request Jun 11, 2026
Open
ralphlange
ralphlange reviewed Jun 12, 2026
View reviewed changes
Comment thread ansible/roles/m_base/tasks/build_module.yml

@ralphlange ralphlange Jun 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is unrelated to Molecule, isn't it?

@simon-ess simon-ess Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not exactly. Molecule testing includes a test that playbooks and roles are idempotent; by its very nature running a shell task cannot be ensured to be idempotent so it will always fail that (unless you have changed_when: false which is not my prefered way of handling things). The idea here is to use some actual ansible modules to perform the same action so that idempotence can pass.

@ralphlange ralphlange Jun 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Well, let me improve my wording, then.

This is not related to adding Molecule, but fixes an issue that Molecule would find, correct?

(So it could be extracted and merged independently. Just trying to understand.)

@simon-ess simon-ess Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah, yes. I suppose it's a general thing when adding tests that you find things that need to be fixed and include them with the changes :D

@ralphlange

ralphlange commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

ERROR Computed fully qualified role name of m-base does not follow current galaxy requirements.

What does that mean?!

@simon-ess

simon-ess commented Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

I have rebased this, but it will take a bit of work to get going correctly. The problem is that these roles are not something that can be run in isolation as they currently depend on variables defined outside of their scope.

I'll see what I can do to clean this up, but it may involve a bit more refactoring of the roles themselves...

@simon-ess

simon-ess commented Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

Regarding the role name, it seems that hyphens are a no-go: https://docs.ansible.com/projects/lint/rules/role-name/

I will change the names of the roles to m_base and whatnot as I modify everything.

@simon-ess simon-ess force-pushed the molecule branch 7 times, most recently from 32082e8 to 1dcd9c7 Compare June 12, 2026 12:06
@simon-ess

simon-ess commented Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

Ok, this passes now and seems to be testing base and the initial setup. It wasn't as bad as I feared!

A few notes to ponder when reviewing this:

  • It does seem to necessitate renaming the roles so they do not have hyphens. I tried skipping that lint and could not get it to work.
  • I have moved some of the variables from ansible/vars into the specific roles that need them, as I think this is more correct. There are distribution variables you use but they are mostly used in a single role, so they should be role variables
  • Similarly, I have dropped the is_redhat and is_debian in favour of directly checking ansible_facts[...] which allows us not to use top-level variables that are not available in the roles.

Testing is still pretty limited, but could of course be expanded. This is more an initial PoC.

@ralphlange

ralphlange commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Ack.

  • Renaming roles is fine. Wish I had known this when I invented those names.
    Why do they allow dashes and then don't?
  • Variables to the roles the use them is fine, too. I thought it was nice to have the distro-related stuff in one place, but no.
  • I saw that you were dropping the distro family shortcuts - well, so much for trying to be more obvious.

@simon-ess

simon-ess commented Jun 15, 2026

Copy link
Copy Markdown
Contributor Author

It's sort of nice, but it makes the roles hard to test on their own since the playbook was setting those variables. As I understand it it is more ansible-idiomatic to directly query the facts, or if necessary for something like is_debian to set it within a role instead of within the playbook.

But as for role names, I'm a bit perplexed by this as well. The roles at ESS are all named something like ics-ans-role-name which has never been an issue; somewhere along the way ansible is more opinionated than it used to be. It is annoying that skipping that lint isn't working.

@ralphlange

ralphlange commented Jun 15, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Should I rename all the roles in a separate commit - and then drop the commit in this PR on rebasing it? Or rename all remaining roles in a separate commit?

@simon-ess

simon-ess commented Jun 15, 2026

Copy link
Copy Markdown
Contributor Author

I'm fine either which way tbh. Your call. I can also rename everything in the commit in this PR...

@ralphlange

ralphlange commented Jun 15, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Would you be willing to do that? Great.
I could understand if you were not - then I'll do it.

@simon-ess

simon-ess commented Jun 15, 2026

Copy link
Copy Markdown
Contributor Author

I'll do it in one mega-commit then 😄

@simon-ess simon-ess force-pushed the molecule branch 2 times, most recently from 65d5bc0 to b3d7fca Compare June 16, 2026 10:51
@simon-ess

simon-ess commented Jun 17, 2026

Copy link
Copy Markdown
Contributor Author

Ok, roles renamed and tests passing again. This could be ready for review/merge now.

Some further thoughts, but maybe they belong in a further set of PRs:

  • Some of the variables from the playbook were moved to these roles as necessary, but there are more that can be moved.
  • The EPICS base tests are pretty light; mostly that several executables are on path. An actual IOC could be spun up and talked to, perhaps
  • Further modules (I would think asyn first, maybe?) could also be given molecule tests

@ralphlange

ralphlange commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Looks good!

One last thing:
Can you reorder/squash the two role-renaming commits? Then I would keep the commits when merging.

@ralphlange

ralphlange commented Jun 17, 2026

Copy link
Copy Markdown
Contributor
  • Further modules (I would think asyn first, maybe?) could also be given molecule tests

Yessir. That's the idea!

@simon-ess

simon-ess commented Jun 18, 2026

Copy link
Copy Markdown
Contributor Author

Looks good!

One last thing: Can you reorder/squash the two role-renaming commits? Then I would keep the commits when merging.

To be clear, you mean split those out so that you can squash this into two commits: renaming the roles, and everything else?

simon-ess added 5 commits June 18, 2026 10:01
@simon-ess
Add tests for m-base
187dcad 
Three simple tests:
* RELEASE.local is created correctly
* softIoc* are on path
* EPICS tools are on path
@simon-ess
Use find/copy instead of command to pass idempotence
e43aa6f
@simon-ess
Move distro-specific variables to relevant role
5776d3b 
The roles themselves should be responsible for the values that they
need if they are to be tested independently; since most of the distro
specific variables are only used in one or two roles, they can be
moved to those roles.

Note that we also add scoping prefixes to the variables to avoid
potential collisions.
@simon-ess
Directly use ansible_facts instead of is_debian/is_redhat
d53841a
@simon-ess
Rename all roles to satisfy ansible role naming convention
5f6f6eb 
Ansible roles should only contain lowercase alphanumeric characters and
underscores.
@ralphlange

ralphlange commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

No.

Just combine the "rename m-base" and "rename all roles" into one commit.

@simon-ess

simon-ess commented Jun 18, 2026

Copy link
Copy Markdown
Contributor Author

No.

Just combine the "rename m-base" and "rename all roles" into one commit.

Done.

@ralphlange ralphlange merged commit ab2dcd0 into epics-training:main Jun 18, 2026
28 checks passed
@ralphlange

ralphlange commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Thanks A LOT for preparing this.
Looking into a bright future with lots of molecules...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ralphlange ralphlange ralphlange left review comments

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

@simon-ess @ralphlange