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

Adding MODULES_SITECONFIG env. variable support #235

Merged
merged 2 commits into from
May 9, 2019

Conversation

nanobowers
Copy link
Contributor

Proposal for issue #234

@codecov
Copy link

codecov bot commented Mar 6, 2019

Codecov Report

Merging #235 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #235      +/-   ##
==========================================
+ Coverage   98.63%   98.64%   +<.01%     
==========================================
  Files           2        2              
  Lines        2423     2427       +4     
==========================================
+ Hits         2390     2394       +4     
  Misses         33       33
Impacted Files Coverage Δ
modulecmd.tcl.in 99.91% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 561918e...75f2d8b. Read the comment docs.

modulecmd.tcl.in Outdated
return @etcdir@/siteconfig.tcl
}

set g_siteconfig [getSiteConfig] ;# Site configuration
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would prefer that getSiteConfig follow the same scheme than getAutoHandling, which means it should set the ::g_siteconfig if it is not set and returns its value in any cases. To set the global variable, it uses the @etcdir@/siteconfig.tclvalue by default or the env(MODULES_SITECONFIG) if 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.

By this, do you also mean that any pieces of downstream code reference [getSiteConfig] rather than referring to $g_siteconfig ?

for example this:

   if {[file readable $g_siteconfig]} {
      reportDebug "Source site configuration ($g_siteconfig)"
      if {[catch {source $g_siteconfig} errMsg]} {

to this:

   if {[file readable [getSiteConfig]]} {
      reportDebug "Source site configuration ([getSiteConfig])"
      if {[catch {source [getSiteConfig]} errMsg]} {

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, exactly.

@xdelaruelle
Copy link
Collaborator

Some non-regression tests should be added to testsuite/modules.00-init/120-siteconfig.exp.

Also the MODULES_SITECONFIG environment variable should be referred in the module.1 man documentation doc/source/module.rst.

@xdelaruelle xdelaruelle added this to the 4.3 milestone Mar 7, 2019
@nanobowers
Copy link
Contributor Author

@xdelaruelle pushed another update, this time with change in priority of sourcing the siteconfig. First will try to source etc/siteconfig.tcl, then will try to source $MODULES_SITECONFIG. Man and tests updated. My previous issue with certain tests failing no longer happens.

@xdelaruelle xdelaruelle force-pushed the siteconfig branch 2 times, most recently from d489bf9 to 5e516b6 Compare May 8, 2019 19:46
@xdelaruelle
Copy link
Collaborator

I have rearranged things a bit, should be good now. Thanks for this contribution.

nanobowers and others added 2 commits May 9, 2019 06:19
Add the ability to define a site-specific configuration file with an
environment variable: MODULES_SITECONFIG. When set, the script file
pointed by the variable is sourced (if readable) after the site-specific
configuration file initially defined in modulecmd.tcl.

Fixes envmodules#234.
@xdelaruelle xdelaruelle merged commit 22495fd into envmodules:master May 9, 2019
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.

2 participants