Add `get_environment_variable` utility function#5148

Open

mvanhorn wants to merge 2 commits intoeasybuilders:developfrom

mvanhorn:osc/5059-get-environment-variable

mvanhorn commented Mar 25, 2026

Summary

Adds get_environment_variable() to easybuild/tools/environment.py for safe environment variable access with optional required and empty validation.

Why this matters

Careless use of os.getenv() without defaults led to None being injected into build commands (#5059, referenced from easyblocks#4003). This function provides a single point of control for environment variable access with clear error messages.

Changes

easybuild/tools/environment.py: Added get_environment_variable(name, required=False, empty=True) with four modes:
- Default: returns value or empty string
- required=True: raises EasyBuildError if undefined
- empty=False: treats empty strings as missing
- Both: raises EasyBuildError if undefined OR empty

Testing

Python syntax verified
Follows existing read_environment() and EasyBuildError patterns

Fixes #5059

This contribution was developed with AI assistance (Claude Code).


          Add get_environment_variable utility function

80d1592

Provides safe environment variable access with required and empty
checks. Returns empty string for undefined variables by default,
raises EasyBuildError when required=True and the variable is
missing or (with empty=False) empty.

Fixes easybuilders#5059

ocaisa requested changes

View reviewed changes

Member

ocaisa left a comment

You will need to add tests for this as well.

easybuild/tools/environment.py Outdated

		return result


		def get_environment_variable(name, required=False, empty=True):

Member

ocaisa Mar 25, 2026

With empty I think what you really mean is allow_empty...and then if we don't allow_empty then it should be a straight error, not dependent on required.

easybuild/tools/environment.py Outdated

+                      _log.debug("Environment variable '%s' is not defined, returning empty string", name)
+                      return ''
+                  if not empty and value == '':

Member

ocaisa Mar 25, 2026

Am empty string is falsey so you can just use value.strip() (which also removes whitespace)

Suggested change

      
                if not empty and value == '':
          
                # Check if we allow empty values (and we consider whitespace an empty value)
          
                if not empty and value.strip():

easybuild/tools/environment.py Outdated

Comment on lines +172 to +173

		if required:
		raise EasyBuildError("Required environment variable '%s' is defined but empty", name)

Member

ocaisa Mar 25, 2026

Suggested change

      
                    if required:
          
                        raise EasyBuildError("Required environment variable '%s' is defined but empty", name)
          
                   raise EasyBuildError("Required environment variable '%s' is defined but empty", name)

easybuild/tools/environment.py Outdated

+                      if required:
+                          raise EasyBuildError("Required environment variable '%s' is not defined", name)
+                      _log.debug("Environment variable '%s' is not defined, returning empty string", name)
+                      return ''

Member

ocaisa Mar 25, 2026

Don't use inline returns

Suggested change

      
                    return ''
          
                    value = ''

easybuild/tools/environment.py Outdated

+                      if required:
+                          raise EasyBuildError("Required environment variable '%s' is defined but empty", name)
+                      _log.debug("Environment variable '%s' is empty, returning empty string", name)
+                      return ''

Member

ocaisa Mar 25, 2026

Suggested change

      
                    return ''
          
                    value = ''

Member

ocaisa commented Mar 25, 2026

Ha, you were working directly off of #5059 but now I think about it, I don't think supporting allow_empty=False with required=False is a good idea in general (you either get '' or None which is basically exactly what you said you didn't want).

Member

boegel commented Mar 25, 2026

I also want to see the adopted, both in framework itself, and in one or more easyblocks, to show the usage of this...

boegel added the AI-assisted label

boegel added this to the 5.x milestone

boegel changed the title ~~Add get_environment_variable utility function~~ Add get_environment_variable utility function


          fix: address review feedback on get_environment_variable utility

1fd1ab3

- Rename `empty` param to `allow_empty` for clarity
- Simplify empty check using `value.strip()` (falsey)
- Remove allow_empty=False + required=False combo (always error on empty)
- Replace inline returns with `value = ''` assignment
- Add test_get_environment_variable covering all modes

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author

mvanhorn commented Mar 25, 2026

Addressed all feedback:

Renamed empty to allow_empty
Simplified check with value.strip() for whitespace handling
Removed inline returns
When allow_empty=False, always raises EasyBuildError regardless of required (removed the confusing combination @ocaisa flagged)
Added tests covering defined/undefined/empty/whitespace-only cases

Still need to add an adoption example in an easyblock per @boegel's request - will follow up on that.

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

Labels