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 no rose vars in cylc view #5367

Merged
merged 3 commits into from
Feb 16, 2023
Merged

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented Feb 15, 2023

This PR closes an issue documented in the PR.

Issue Description

Cylc view does not have access to the Cylc Rose command line options (-O, -S, -D), when it ought to have.

PR Information

I have added the Cylc Rose CLI options to cylc view. It works manually, but ought to be tested in Cylc Rose.

Question - do we want to test the Cylc Rose option adder in this repo by looking at the --help output?

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.
  • Tests are not included because they should be in Cylc Rose; Add tests for rose options being available in Cylc View cylc-rose#209
  • CHANGES.md entry not included - this is not a change that can affect users
  • Cylc doc update considered - not required.
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

@wxtim wxtim self-assigned this Feb 15, 2023
@wxtim wxtim added bug small question Flag this as a question for the next Cylc project meeting. labels Feb 15, 2023
@wxtim wxtim added this to the cylc-8.1.2 milestone Feb 15, 2023
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.

LGTM

CHANGES.md Outdated Show resolved Hide resolved
@oliver-sanders
Copy link
Member

Question - do we want to test the Cylc Rose option adder in this repo by looking at the --help output?

Can do that in the cylc-rose repo where there should be some e2e tests for this?

Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>
@wxtim
Copy link
Member Author

wxtim commented Feb 15, 2023

Question - do we want to test the Cylc Rose option adder in this repo by looking at the --help output?

Can do that in the cylc-rose repo where there should be some e2e tests for this?

See the link against the "tests" checklist item.

@oliver-sanders oliver-sanders removed the question Flag this as a question for the next Cylc project meeting. label Feb 15, 2023
@oliver-sanders oliver-sanders requested review from oliver-sanders and removed request for datamel February 16, 2023 12:46
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.

wrong branch

If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

Please finish checklist.

@wxtim wxtim changed the base branch from master to 8.1.x February 16, 2023 14:16
@wxtim
Copy link
Member Author

wxtim commented Feb 16, 2023

All failures caused by hitting codecov rate limits.

@wxtim wxtim merged commit 1584fd9 into cylc:8.1.x Feb 16, 2023
@wxtim wxtim deleted the fix_no_rose_vars_in_cylc_view branch February 16, 2023 14:51
wxtim added a commit to wxtim/cylc that referenced this pull request Feb 17, 2023
…lc into fix_localhost_platform_matching

* 'fix_localhost_platform_matching' of github.com:wxtim/cylc:
  Fix a bug preventing `cylc vip --workflow-name=foo` from working. (cylc#5349)
  fix no rose vars in cylc view (cylc#5367)
  Cylc lint fixes (cylc#5363)
  data store: support unsatisfied ext_trigger
  fix mypy fail caused by python/mypy#13969
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants