Skip to content

Re-order env-vars so they are set after env-vars set in dependencies#51

Merged
ptsOSL merged 2 commits intomainfrom
hla-1044-allow-modules-to-override-dependency-env-vars
Apr 24, 2026
Merged

Re-order env-vars so they are set after env-vars set in dependencies#51
ptsOSL merged 2 commits intomainfrom
hla-1044-allow-modules-to-override-dependency-env-vars

Conversation

@ptsOSL
Copy link
Copy Markdown
Collaborator

@ptsOSL ptsOSL commented Apr 23, 2026

It seems sensible that the env-vars set by dependencies would be overwritten by the modulefile for the module you are loading.

Giles did highlight that there may be situations where your dependencies require env-vars defining before they are module loaded, such as when env-vars are needed to load the module correctly. But currently he is not doing this.

@ptsOSL ptsOSL requested review from MJGaughran and gilesknap April 23, 2026 10:44
@ptsOSL ptsOSL self-assigned this Apr 23, 2026
@MJGaughran
Copy link
Copy Markdown
Contributor

I'm not aware of anything that would require environment variables to be set before loading; I suspect that would be considered a poorly-constructed modulefile, as they should be including any dependencies (including env vars) automatically when you load them.

But @gilesknap, if you have any examples from DLS that would be appreciated.

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 23, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.64%. Comparing base (2f5d1b5) to head (2d84cae).

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #51   +/-   ##
=======================================
  Coverage   75.64%   75.64%           
=======================================
  Files          25       25           
  Lines         936      936           
=======================================
  Hits          708      708           
  Misses        228      228           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

MJGaughran
MJGaughran previously approved these changes Apr 23, 2026
@ptsOSL ptsOSL force-pushed the hla-1044-allow-modules-to-override-dependency-env-vars branch 4 times, most recently from 98749e9 to 2d84cae Compare April 23, 2026 16:15
@gilesknap
Copy link
Copy Markdown
Contributor

@MJGaughran no I don't know of any modules that require environment set when loading. As you say that would not be a great idea anyway.

@ptsOSL ptsOSL force-pushed the hla-1044-allow-modules-to-override-dependency-env-vars branch from 2d84cae to c1a6994 Compare April 24, 2026 09:24
@ptsOSL ptsOSL merged commit 061ecd6 into main Apr 24, 2026
21 checks passed
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