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

Refactor the CI configuration to use inclusion instead of exclusion #3616

Merged
merged 4 commits into from
Mar 31, 2021

Conversation

Flamefire
Copy link
Contributor

This provides more clarity on what exactly is run

The jobs have not changed, the following configurations are tested before (and after):

build (2.7, Lmod-7.8.22, Lua)
build (2.7, Lmod-7.8.22, Tcl)
build (2.7, Lmod-8.2.9, Lua)
build (2.7, Lmod-8.2.9, Tcl)
build (2.7, modules-tcl-1.147, Tcl)
build (2.7, modules-3.2.10, Tcl)
build (2.7, modules-4.1.4, Tcl)

build (3.6, Lmod-7.8.22, Lua)
build (3.6, Lmod-7.8.22, Tcl)
build (3.6, Lmod-8.2.9, Lua)
build (3.6, Lmod-8.2.9, Tcl)
build (3.6, modules-tcl-1.147, Tcl)
build (3.6, modules-3.2.10, Tcl)
build (3.6, modules-4.1.4, Tcl)

build (3.5, Lmod-8.2.9, Lua)
build (3.5, Lmod-8.2.9, Tcl)

build (3.7, Lmod-8.2.9, Lua)
build (3.7, Lmod-8.2.9, Tcl)

build (3.8, Lmod-8.2.9, Lua)
build (3.8, Lmod-8.2.9, Tcl)

build (3.9, Lmod-8.2.9, Lua)
build (3.9, Lmod-8.2.9, Tcl)

build (3.6, Lmod-8.2.9, Lua, C)

This provides more clarity on what exactly is run
boegel
boegel previously requested changes Mar 18, 2021
Copy link
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

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

@Flamefire lgtm, that looks a bit cleaner indeed.
Let's bump to the latest Lmod 8.x while we're at it: Flamefire#7

@Flamefire
Copy link
Contributor Author

Flamefire commented Mar 18, 2021

I'd like to try something more: Avoid the repetition of the lmod version.
It seems you can generate a whole matrix from within a job step: https://docs.github.com/en/actions/reference/context-and-expression-syntax-for-github-actions#example-returning-a-json-object

But what we just need here is something we can access with ${{}}, e.g. an env variable.
I'll try that later, so hold up the merge please :)

@boegel boegel dismissed their stale review March 18, 2021 21:34

Lmod version bumped

@boegel
Copy link
Member

boegel commented Mar 18, 2021

I'd like to try something more: Avoid the repetition of the lmod version.
It seems you can generate a whole matrix from within a job step: https://docs.github.com/en/actions/reference/context-and-expression-syntax-for-github-actions#example-returning-a-json-object

But what we just need here is something we can access with ${{}}, e.g. an env variable.
I'll try that later, so hold up the merge please :)

OK! :)

@boegel
Copy link
Member

boegel commented Mar 19, 2021

@Flamefire Once you're happy with the changes, I'll need to re-configure the test configurations that are required to pass (which is why there's a bunch of orange dots sitting there "waiting" right now).

@Flamefire Flamefire force-pushed the improve_gha branch 2 times, most recently from b2ae014 to 69ce2f7 Compare March 19, 2021 11:27
@Flamefire
Copy link
Contributor Author

I think this works now. What do you think about this approach?
Alternative:

env:
  LMOD_7: Lmod-...
  ...
  modules_tool: [${env.LMOD_7}, ...]

That's a bit shorter but sets those as actual env variables, so my current approach feels "more right"

Copy link
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

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

lgtm

@boegel boegel changed the title Refactor the CI script to use inclusion instead of exclusion Refactor the CI configuration to use inclusion instead of exclusion Mar 31, 2021
@boegel boegel merged commit f215ae5 into easybuilders:develop Mar 31, 2021
@Flamefire Flamefire deleted the improve_gha branch March 31, 2021 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants