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

Permitted warnings about root-dir for all versions of Cylc. #231

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented Jun 13, 2023

Pull request closing an issue documented here:

Issue

This issue (#230) describes a rose suite configuration which raises warnings when cylc compatibility mode is turned off. The warning that root-dir is no longer valid at Cylc 8 is valid whether or not compatibility mode is turned on and should be shown in either case.

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
  • Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

@wxtim wxtim force-pushed the fix.root-dir_warning_applies_irrespective_of_compat_mode branch from c3f7646 to c1859f4 Compare June 13, 2023 15:19
@wxtim wxtim self-assigned this Jun 13, 2023
@wxtim wxtim added bug Something isn't working small labels Jun 13, 2023
@wxtim wxtim added this to the 1.2.1 milestone Jun 13, 2023
@wxtim
Copy link
Member Author

wxtim commented Jun 14, 2023

This is also broken on 1.2.x. https://github.com/cylc/cylc-rose/actions/runs/5266262439

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.

Tested with cylc/cylc-flow#5582, Rose warnings show as expected.

@@ -42,6 +42,8 @@
ROSE_ORIG_HOST_INSTALLED_OVERRIDE_STRING = (
' ROSE_ORIG_HOST set by cylc install.'
)
MESSAGE = 'message'
Copy link
Member

Choose a reason for hiding this comment

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

FYI, if you find yourself coding dictionary keys as constants, then named tuples are likely a better fit.

@oliver-sanders
Copy link
Member

oliver-sanders commented Jun 14, 2023

Note this contains a commit under review on #232, please merge after.

Merged.

tests/conftest.py Outdated Show resolved Hide resolved
@oliver-sanders
Copy link
Member

@wxtim, I made a mistake deconflicting this, with the diff in the comment above this should work.

Copy link
Member

@MetRonnie MetRonnie left a comment

Choose a reason for hiding this comment

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

I've sorted out the merge issue. Awaiting tests passing

@oliver-sanders oliver-sanders merged commit ca95d8e into cylc:1.2.x Jul 17, 2023
5 checks passed
wxtim added a commit to wxtim/cylc-rose that referenced this pull request Oct 9, 2023
…ow_--defines_no_equal

* upstream/1.3.x:
  make RoseStemVersionException print the correct missing variable (cylc#252)
  Update tests/unit/test_rose_stem_units.py
  Allow top level  settings in CLI Config (cylc#221)
  Prevent accidental manual setting of project name with -s=
  Fix flake8
  Bump dev version
  Add missing dependency on ansimarkup
  Prepare release 1.3.0
  Bump pypa/gh-action-pypi-publish from 1.8.7 to 1.8.8 (cylc#236)
  Permitted warnings about root-dir for all versions of Cylc. (cylc#231)
  Bump pypa/gh-action-pypi-publish from 1.8.6 to 1.8.7 (cylc#235)
  Bump pypa/gh-action-pypi-publish from 1.8.5 to 1.8.6 (cylc#228)
  1.3.0 (cylc#219)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants