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

Fix.cylc rose issue 319 #322

Merged
merged 2 commits into from
May 14, 2024
Merged

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented May 10, 2024

Works with cylc/cylc-flow#6097 to close #319.

Functional test provided by metomi/rose#2779

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 self-assigned this May 10, 2024
@wxtim wxtim marked this pull request as draft May 10, 2024 13:56
@wxtim wxtim requested a review from oliver-sanders May 10, 2024 14:13
@wxtim wxtim changed the base branch from master to 1.3.x May 10, 2024 14:13
@wxtim wxtim marked this pull request as ready for review May 10, 2024 14:14
@wxtim wxtim added this to the 1.3.x milestone May 10, 2024
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.

Looks good, possible simplification.

cylc/rose/platform_utils.py Outdated Show resolved Hide resolved
cylc/rose/platform_utils.py Outdated Show resolved Hide resolved
@wxtim wxtim requested a review from oliver-sanders May 13, 2024 07:10
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.

Note, the issue is tagged against 1.4.0 not 1.3.x. We can leave this on the bugfix milestones, but will need to amend the cylc-flow dependency in setup.cfg to prevent this from being used with an incompatible release of cylc-flow if we were to make another bugfix release before the cylc 8.3 meta-release.

@wxtim wxtim changed the base branch from 1.3.x to master May 13, 2024 09:29
@wxtim wxtim modified the milestones: 1.3.x, 1.4.0 May 13, 2024
@wxtim wxtim requested a review from oliver-sanders May 13, 2024 09:30
@oliver-sanders
Copy link
Member

(tests will fail until upstream PR merged)

@wxtim wxtim requested a review from MetRonnie May 13, 2024 10:18
@oliver-sanders
Copy link
Member

Could do with a quick rebase now that the sync PR is in.

Comment on lines +86 to +94
def get_compat_mode(run_dir: Union[str, Path]) -> bool:
"""Check whether this is a Cylc 7 Back compatibility mode workflow:

See https://github.com/cylc/cylc-rose/issues/319

Args:
run_dir: Cylc workflow run directory.
"""
return (Path(run_dir) / WorkflowFiles.SUITE_RC).exists()
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't we just import and use cylc.flow.workflow_files.check_deprecation?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess that we could, but I'd rather not risk change the state of the cylc.flow.flags.cylc7_back_compat flag if at all possible: This approach means that information is only really flowing from Cylc flow into Cylc Rose.

@wxtim wxtim requested a review from MetRonnie May 13, 2024 12:43
wxtim added 2 commits May 13, 2024 13:44
…ening the flow-processed.cylc file, and override the compat mode in the workflow config if required.
@MetRonnie
Copy link
Member

Kicking tests

@MetRonnie MetRonnie closed this May 13, 2024
@MetRonnie MetRonnie reopened this May 13, 2024
@MetRonnie MetRonnie merged commit 244c850 into cylc:master May 14, 2024
10 of 16 checks passed
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.

compat mode ignored
3 participants