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

Resolving shell environment variables name space conflicts with parm #28

Conversation

TerrenceMcGuinness-NOAA
Copy link
Contributor

@TerrenceMcGuinness-NOAA TerrenceMcGuinness-NOAA commented Jun 3, 2024

Description

This PR updates the instantiation of the Configuration Class of wxflow in order to resolve the namespaces clashes between the environment variables in shell that launches setup_xml.py and the values used in config parm variables.

With this update the namespaces between the shell environment variables and the variables in parm files used by the Configuration Class are preserved. A Unit Test demonstrating this can be found here.

For example, if the environment variable ACCOUNT is set in the shell that setup_expt.py is launched from, it would would fail as that variable is also used in template of config.base. Correspondingly, if setup_xml.py was executed with ACCOUNT set in the shell it would populate the XML file with UNKOWN without erroring. With this update the value in the XML file would be set he value designated in the corresponding instantiated parm file config.base.

The Unit Tests specified below shows this failure does not occure and the updates are used in Test PR 2581

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

  • pynorms
  • pytests

This PR Branch was incorporated into Global-Workflow PR 2581 Unit Tests using GitHub Actions.
Here are the results of running the Unit Tests from GitHub Actions. PR 2581 also does full CI run throughs using this update in the Configuration Class.

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes need updates to the documentation. I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • New and existing tests pass with my changes
  • Any dependent changes have been merged and published

Copy link

codecov bot commented Jun 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 50.06%. Comparing base (5dad7dd) to head (5382a0e).
Report is 6 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop      #28      +/-   ##
===========================================
+ Coverage    48.23%   50.06%   +1.82%     
===========================================
  Files           18       18              
  Lines         1644     1648       +4     
  Branches       335      336       +1     
===========================================
+ Hits           793      825      +32     
+ Misses         791      763      -28     
  Partials        60       60              

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

Copy link
Contributor

@aerorahul aerorahul left a comment

Choose a reason for hiding this comment

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

looks good.

Copy link
Contributor

@aerorahul aerorahul left a comment

Choose a reason for hiding this comment

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

please activate the tests that are marked to skip in the GH runner.

@TerrenceMcGuinness-NOAA TerrenceMcGuinness-NOAA marked this pull request as draft June 6, 2024 12:46
@TerrenceMcGuinness-NOAA TerrenceMcGuinness-NOAA marked this pull request as ready for review June 6, 2024 14:38
@TerrenceMcGuinness-NOAA
Copy link
Contributor Author

TerrenceMcGuinness-NOAA commented Jun 6, 2024

please activate the tests that are marked to skip in the GH runner.

@aerorahul Done: Configuration Unit Tests now pass in GitHub Runners with the update to the bash shell in the Subprocess. Also I updated the approach that solves the primary name classing issue with feedback from these tests that is now also more strait forward than before. Please re-review these changes. Thnx. ~T.McG

Comment on lines 101 to 105
keys_in_scripts = set()
regex_pattern = 'export\\s+\\b(' + '|'.join(map(re.escape, default_env.keys())) + ')(?==)'
for script in scripts:
result = subprocess.run(['grep', '-o', '-P', regex_pattern, script], stdout=subprocess.PIPE)
keys_in_scripts.update(result.stdout.decode().split())
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be explained a bit?
The task is simple here; get a dictionary of key-value pairs set by a shell script.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The old approach was faulty, this one (not intended to be used) resorted to using a regular expression to fish out any environment variable that is in the user spaces that is also in a config file. It is here only to show what is needed to pass the unit tests. I don't think this is good solution, so I'm thinking of another approach entirely to avoid chicken-before-egg dilemma that this will not solve.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes and then just check if the key is in both!

@aerorahul
Copy link
Contributor

Not required by author.

@aerorahul aerorahul closed this Jul 11, 2024
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.

4 participants