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.

Sign up for GitHub

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

Jump to bottom

feature: stem installs to workflow name not rose-stem #180

Merged
merged 4 commits into from
Oct 21, 2022
Merged

feature: stem installs to workflow name not rose-stem #180

merged 4 commits into from
Oct 21, 2022

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented Sep 13, 2022 •
edited
Loading

These changes close #177

Requirements 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 setup.cfg.
  • Appropriate tests are included (unit and/or functional).
  • Appropriate change log entry included.
  • No documentation update required.

@wxtim wxtim self-assigned this Sep 13, 2022
@wxtim wxtim marked this pull request as draft September 13, 2022 09:38
@wxtim wxtim changed the title 20220913 t1038 feature.stem installs to workflow name where workflow name is basename of stem suite feature: stem installs to workflow name not rose-stem Sep 13, 2022
@wxtim wxtim changed the base branch from master to 1.1.x September 13, 2022 09:50
@wxtim wxtim force-pushed the 20220913T1038--feature.stem_installs_to_workflow_name_where_workflow_name_is_basename_of_stem_suite branch from 65fc3e8 to ba54ba1 Compare September 13, 2022 12:08
@wxtim wxtim added this to the 1.1.2 milestone Sep 13, 2022
@wxtim wxtim added the enhancement New feature or request label Sep 13, 2022
@wxtim wxtim requested review from datamel and dpmatthews September 13, 2022 12:10
@wxtim wxtim modified the milestones: 1.1.2, 1.1.1 Sep 13, 2022
@wxtim wxtim force-pushed the 20220913T1038--feature.stem_installs_to_workflow_name_where_workflow_name_is_basename_of_stem_suite branch 2 times, most recently from 5928fd4 to 3584c4f Compare September 13, 2022 12:15
@wxtim wxtim marked this pull request as ready for review September 13, 2022 12:24
@wxtim wxtim linked an issue Sep 13, 2022 that may be closed by this pull request
Rose stem installs to ~/cylc-run/rose-stem #177
Closed
@wxtim wxtim mentioned this pull request Sep 14, 2022
Closed
@oliver-sanders oliver-sanders changed the title feature: stem installs to workflow name not rose-stem bug: stem installs to workflow name not rose-stem Sep 14, 2022
@oliver-sanders oliver-sanders added bug Something isn't working and removed enhancement New feature or request labels Sep 14, 2022
@oliver-sanders
Copy link
Member

Pretty sure this is a bug right (otherwise it goes in 1.2.0), swapped "enhancement" for "bug" to match the issue.

@wxtim wxtim added enhancement New feature or request and removed bug Something isn't working labels Sep 14, 2022
@wxtim wxtim modified the milestones: 1.1.1, 1.2.0 Sep 14, 2022
@wxtim
Copy link
Member Author

wxtim commented Sep 14, 2022

Pretty sure this is a bug right (otherwise it goes in 1.2.0), swapped "enhancement" for "bug" to match the issue.

It's the same behaviour as Rose 2019.

@wxtim wxtim modified the milestones: 1.2.0, 1.1.2 Sep 14, 2022
@wxtim wxtim added bug Something isn't working and removed enhancement New feature or request labels Sep 14, 2022
@oliver-sanders oliver-sanders removed the bug Something isn't working label Sep 14, 2022
@oliver-sanders oliver-sanders added the enhancement New feature or request label Sep 14, 2022
datamel
datamel approved these changes Sep 20, 2022
View reviewed changes
Copy link
Contributor

@datamel datamel left a comment

Choose a reason for hiding this comment

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

I have run the tests on master and they fail, they pass locally for me on this branch. I have also read the code and manually tested this change, using the documented command in the issue.
Testing this out, I had a play around and rose stem is not coping with source with a ~, expanding it myself is working. Would this be a desirable feature for our users? If so I can open another issue.

@oliver-sanders oliver-sanders changed the title bug: stem installs to workflow name not rose-stem featue: stem installs to workflow name not rose-stem Sep 21, 2022
@oliver-sanders oliver-sanders changed the title featue: stem installs to workflow name not rose-stem feature: stem installs to workflow name not rose-stem Sep 21, 2022
dpmatthews
dpmatthews previously requested changes Sep 29, 2022
View reviewed changes
cylc/rose/stem.py Outdated Show resolved Hide resolved
@wxtim wxtim requested review from dpmatthews and datamel September 30, 2022 07:14
@wxtim wxtim force-pushed the 20220913T1038--feature.stem_installs_to_workflow_name_where_workflow_name_is_basename_of_stem_suite branch from 0b0b88a to 0ab9119 Compare October 4, 2022 07:48
datamel
datamel approved these changes Oct 10, 2022
View reviewed changes
Copy link
Contributor

@datamel datamel left a comment

Choose a reason for hiding this comment

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

LGTM. I have checked this out (rebased to get latest setup.cfg), run tests locally and used the example in the issue to test it is working as expected.

@wxtim wxtim requested a review from oliver-sanders October 11, 2022 14:22
@oliver-sanders oliver-sanders removed their request for review October 11, 2022 15:12
@wxtim wxtim requested a review from MetRonnie October 14, 2022 07:33
MetRonnie
MetRonnie reviewed Oct 18, 2022
View reviewed changes
cylc/rose/stem.py Outdated Show resolved Hide resolved
cylc/rose/stem.py Outdated Show resolved Hide resolved
cylc/rose/stem.py Outdated Show resolved Hide resolved
cylc/rose/stem.py Outdated Show resolved Hide resolved
cylc/rose/stem.py Outdated Show resolved Hide resolved
wxtim
wxtim commented Oct 18, 2022
View reviewed changes
cylc/rose/stem.py Outdated Show resolved Hide resolved
@wxtim wxtim requested a review from MetRonnie October 18, 2022 14:01
@wxtim wxtim dismissed dpmatthews’s stale review October 21, 2022 09:23

Left the conversation

@wxtim wxtim merged commit 9974e2b into cylc:1.1.x Oct 21, 2022
@wxtim wxtim deleted the 20220913T1038--feature.stem_installs_to_workflow_name_where_workflow_name_is_basename_of_stem_suite branch October 21, 2022 09:24
@oliver-sanders oliver-sanders modified the milestones: 1.1.2, 1.2.0 Jan 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@datamel datamel datamel approved these changes

@MetRonnie MetRonnie MetRonnie approved these changes

@dpmatthews dpmatthews Awaiting requested review from dpmatthews

Assignees

@wxtim wxtim

Labels
enhancement New feature or request
Projects
None yet
Milestone
1.2.0
Development

Successfully merging this pull request may close these issues.

Rose stem installs to ~/cylc-run/rose-stem
5 participants
@wxtim @oliver-sanders @dpmatthews @datamel @MetRonnie