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

Rose Stem Fixes #172

Merged
merged 3 commits into from
Sep 12, 2022
Merged

Rose Stem Fixes #172

merged 3 commits into from
Sep 12, 2022

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented Aug 26, 2022 •
edited

This is a small change with no associated Issue.

Summary of issue

At Rose 2019 the command rose stem --source=X=$PWD would set the variable conf_dir to None by default, before passing the options to the StemRunner.process method.

At Rose 2 I had failed to do this and as a result attempts to calculate the conf_dir in Rose failed because there was no conf_dir to reference.

n.b. In the process of translating the Rose 2019 code I renamed conf_dir and source to source and stem_sources respectively. I'm not completely happy with that decision, but I don't want to add a reversion to it to this PR.

Additional things

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 force-pushed the fix.rose-stem-sourceis-None branch from b86cdd2 to 9d66ff1 Compare August 26, 2022 12:31
datamel
datamel approved these changes Aug 31, 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.

Checked out locally, tests run and compared this behaviour on master. This works as detailed in the description. Thanks @wxtim.

@oliver-sanders oliver-sanders added the bug Something isn't working label Aug 31, 2022
@oliver-sanders oliver-sanders added this to the 1.1.1 milestone Aug 31, 2022
@wxtim wxtim mentioned this pull request Aug 31, 2022
Closed
7 tasks
@datamel
Copy link
Contributor

datamel commented Aug 31, 2022 •
edited

Just to note that this is still the case: #170 (review)

So, either:

  • As this was not supported in previous version, the help needs updating as it currently reads:
# Install a rose-stem suite from PWD/relative-path.
$ rose stem relative-path  #<<<< This does not work

# Install a rose-stem suite from specified abs path.
$ rose stem /absolute/path  #<<<< This does not work
  • or, absolute path needs supporting/fixing.

This may be suitable for a different ticket.

@MetRonnie MetRonnie changed the base branch from master to 1.1.x August 31, 2022 15:25
@wxtim wxtim force-pushed the fix.rose-stem-sourceis-None branch from 9d66ff1 to 8860884 Compare August 31, 2022 15:33
datamel
datamel approved these changes Sep 6, 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'm happy with the updated help. The comment about the absolute path causing error is only the case for non-svn sources and so the help update resolves that comment from me. Thanks @wxtim.

@wxtim
Ensured that opts.source is not unset when opts is passed to the Stem…
39339e7 
…Runner object

update the help docs to match Rose 2019

fix broken test

increase or decrease the verbosity of the logging.
@wxtim wxtim force-pushed the fix.rose-stem-sourceis-None branch from 4b5d0cb to 39339e7 Compare September 7, 2022 12:17
@wxtim wxtim changed the title Ensured that opts.source is not unset when opts is passed to the StemRunner object Assorted Rose Stem Fixes Sep 7, 2022
@wxtim wxtim requested a review from datamel September 7, 2022 12:21
@datamel datamel self-requested a review September 8, 2022 08:09
@wxtim wxtim changed the title Assorted Rose Stem Fixes Rose Stem Fixes Sep 9, 2022
dpmatthews
dpmatthews approved these changes Sep 12, 2022
View reviewed changes
Copy link

@dpmatthews dpmatthews left a comment

Choose a reason for hiding this comment

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

Fixes the problem in my tests

datamel
datamel approved these changes Sep 12, 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.

Thanks @wxtim

@datamel datamel merged commit c596df3 into cylc:1.1.x Sep 12, 2022
@wxtim wxtim deleted the fix.rose-stem-sourceis-None branch September 12, 2022 16:56
wxtim added a commit to wxtim/cylc-rose that referenced this pull request Sep 13, 2022
@wxtim
Rose Stem Fixes (cylc#172)
591c245 
* Ensured that opts.source is not unset when opts is passed to the StemRunner object

update the help docs to match Rose 2019

fix broken test

increase or decrease the verbosity of the logging.

* fix stuff

* fixed opts.verbose
@wxtim wxtim mentioned this pull request Sep 14, 2022
Closed
wxtim added a commit to wxtim/cylc-rose that referenced this pull request Nov 25, 2022
@wxtim
Rose Stem Fixes (cylc#172)
1cc6772 
* Ensured that opts.source is not unset when opts is passed to the StemRunner object

update the help docs to match Rose 2019

fix broken test

increase or decrease the verbosity of the logging.

* fix stuff

* fixed opts.verbose
wxtim added a commit to wxtim/cylc-rose that referenced this pull request Nov 25, 2022
@wxtim
Merge branch 'master' of github.com:cylc/cylc-rose into further_incre…
317a9c0 
…mental_improvements_to_testing

* 'master' of github.com:cylc/cylc-rose:
  Remove subprocess from tests (cylc#194)
  update stem - install interface (cylc#193)
  Update tests/unit/test_rose_stem_units.py
  response to review
  Update setup.cfg
  removed never used option
  Improve test coverage for Rose Stem
  reinstall changes to `rose-suite.conf` [tests] (cylc#178)
  Changed the rose stem functional tests to a more pytest-integration style.
  Bump rose dependency to `2.1.*`
  Bump dev version on master to next minor release (cylc#175)
  Fix changelog conflict with 1.1.x (cylc#186)
  Prepare release: 1.1.1 (cylc#183)
  Rose Stem Fixes (cylc#172)
  Don't pass rose variables with state ! or !! to Cylc. (cylc#171)
  Don't pass rose variables with state ! or !! to Cylc. (cylc#171)
  Bump cylc-flow dependency (cylc#173)
  functional tests only cleanup if succeeded
@MetRonnie MetRonnie mentioned this pull request Jul 21, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dpmatthews dpmatthews dpmatthews approved these changes

@datamel datamel datamel approved these changes

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
1.1.1
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@wxtim @datamel @dpmatthews @oliver-sanders