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

Repopulate cached GlobalConfig object if CYLC_SYMLINKS exported to env. #269

Merged

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented Dec 4, 2023

Closes #237

This is now ready for review but needs testing against cylc/cylc-flow#5839.

Ensure that the cached globalconfig object is reloaded after the export of CYLC_SYMLINKS variable. This means that using the CYLC_SYMLINKS variable allows users to specify installation symlink locations in the rose-suite.conf.

n.b. I was surprised to find that running glbl_cfg()._DEFAULT = None did not clear the cached config in a way that would force the reload in Cylc, but I think just reloading the config in Cylc Rose makes at least as much sense.

Check List

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Applied any dependency changes to both setup.cfg (and conda-environment.yml if present).
  • Tests are included (or explain why tests are not needed).
  • CHANGES.md entry included if this is a change that can affect users
  • Docs: I have updated the plugin documentation.
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

@wxtim wxtim self-assigned this Dec 4, 2023
@wxtim wxtim added the enhancement New feature or request label Dec 4, 2023
@wxtim wxtim added this to the 1.4.0 milestone Dec 4, 2023
@wxtim wxtim force-pushed the feat.reload_glbl_cfg_after_export_CYLC_SYMLINKS branch 2 times, most recently from 4a34384 to 81dec2c Compare December 4, 2023 11:29
@wxtim wxtim requested a review from MetRonnie December 4, 2023 11:30
cylc/rose/utilities.py Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
cylc/rose/__init__.py Show resolved Hide resolved
@wxtim wxtim marked this pull request as draft December 8, 2023 11:29
@wxtim
Copy link
Member Author

wxtim commented Dec 8, 2023

This branch is broken and not ready to be merged.

~🚧 🏗️ 🚧 ~

Now only failing the same as master

@wxtim wxtim requested a review from MetRonnie December 11, 2023 13:25
@wxtim wxtim marked this pull request as ready for review December 11, 2023 13:25
@wxtim wxtim force-pushed the feat.reload_glbl_cfg_after_export_CYLC_SYMLINKS branch 2 times, most recently from 5d4e8ed to 0cc308d Compare December 11, 2023 13:47
@wxtim wxtim marked this pull request as draft January 2, 2024 10:01
is reloaded after the export of `CYLC_SYMLINKS` variable.
This means that using the `CYLC_SYMLINKS` variable allows
users to specify installation symlink locations in the
`rose-suite.conf`.
@wxtim wxtim force-pushed the feat.reload_glbl_cfg_after_export_CYLC_SYMLINKS branch from 0cc308d to fa3611b Compare January 2, 2024 10:38
Added module version of gen workflow name fixture.
Fix refs to runN or run1.
@wxtim wxtim force-pushed the feat.reload_glbl_cfg_after_export_CYLC_SYMLINKS branch from 3bc9b27 to 6ff23bf Compare January 2, 2024 11:40
@wxtim
Copy link
Member Author

wxtim commented Jan 2, 2024

Now failing identically to master nightly tests.

@wxtim wxtim marked this pull request as ready for review January 2, 2024 11:42
Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

LGTM

tests/conftest.py Outdated Show resolved Hide resolved
tests/functional/test_utils.py Outdated Show resolved Hide resolved
tests/functional/test_utils.py Outdated Show resolved Hide resolved
tests/functional/test_utils.py Show resolved Hide resolved
Co-authored-by: Oliver Sanders <[email protected]>

Update tests/functional/test_utils.py

Co-authored-by: Oliver Sanders <[email protected]>

fix tests
@wxtim wxtim force-pushed the feat.reload_glbl_cfg_after_export_CYLC_SYMLINKS branch from 421782e to 6bb14bb Compare January 3, 2024 15:03
cylc/rose/__init__.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
tests/functional/test_utils.py Outdated Show resolved Hide resolved
wxtim and others added 3 commits January 15, 2024 13:00
…hub.com:wxtim/cylc-rose into feat.reload_glbl_cfg_after_export_CYLC_SYMLINKS

* 'feat.reload_glbl_cfg_after_export_CYLC_SYMLINKS' of github.com:wxtim/cylc-rose:
  Apply suggestions from code review
@wxtim wxtim requested a review from MetRonnie January 15, 2024 13:09
@wxtim
Copy link
Member Author

wxtim commented Jan 24, 2024

@MetRonnie - this is failing in the same way as master - I'm working on fixing in #289, but I don't think that this PR needs to away that one for review.

@wxtim wxtim merged commit a90198c into cylc:master Feb 6, 2024
2 of 7 checks passed
@wxtim wxtim deleted the feat.reload_glbl_cfg_after_export_CYLC_SYMLINKS branch February 6, 2024 14:39
# the global config so that the value of this vars
# can be used by Jinja2 in the global config.
# https://github.com/cylc/cylc-rose/issues/237
if environment:
Copy link
Member

Choose a reason for hiding this comment

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

Note, the environment is always populated as a result of automatic Rose variables.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Environment variables set in rose-suite.conf should apply when the global config is loaded
3 participants