-
Notifications
You must be signed in to change notification settings - Fork 93
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
Cli setting of rose opts #4023
Cli setting of rose opts #4023
Conversation
ef70293
to
d6a5461
Compare
Sorry if I'm missing some context here, but would be the rationale of porting over |
@TomekTrzeciak responded on the issue - #3819 (comment) |
$ cylc install .
PluginError: Error in plugin cylc.pre_configure.rose
'NoneType' object has no attribute 'opt_conf_keys' |
Ok, got it. The
|
@oliver-sanders I think I've excluded |
Conflicted after merge with the cylc-install origin branch. Therefore tests aren't running. |
Due to the way that this branch is effectively built on top of @datamel 's my merges to keep it updated have resulted in a horrid looking history. Given that the final diff is likely to be fairly small it's my intention to do a git diff with master once the |
Could try squashing your commits then you can cherry-pick them/it on top. |
Yep, sorry my cylc-install history looks so terrible! Should look a bit better now I have rebased. |
7e2e00c
to
ce0baa9
Compare
ce0baa9
to
4e9dd74
Compare
Outstanding test failure will stop failing when cylc-rose PR20 is merged to master. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have read through the code and had a play around with it. I am happy with this PR, although I think consider adding functional tests cylc-flow side.
1a8e5e6
to
eda0b0c
Compare
c95f2ab
to
cb21b2b
Compare
Tests passing with cylc-rose PR20 with latest commits. |
export ROSE_FILES='' | ||
python -c "import cylc.rose" > /dev/null 2>&1 && | ||
export ROSE_FILES=" | ||
|-- opt | ||
| \`-- rose-suite-cylc-install.conf | ||
|-- rose-suite.conf" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested with and without cylc-rose installed, all good.
export ROSE_FILES='' | ||
if python -c "import cylc.rose" > /dev/null 2>&1; then | ||
export ROSE_FILES="├── log | ||
│ └── install | ||
├── opt | ||
│ └── rose-suite-cylc-install.conf | ||
└── rose-suite.conf" | ||
else | ||
export ROSE_FILES="└── log | ||
└── install" | ||
fi | ||
|
||
tree -a -v -I '*.log|03-file-transfer*' --charset UTF8 --noreport "${RND_SUITE_RUNDIR}/" > 'basic-tree.out' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested with and without cylc-rose all good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good @datamel could you give this a quick once-over.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have re-run the tests and read through the code changes since the last review. LGTM, thanks @wxtim
built on: #4000
sibling: cylc-rose PR20
Summary
These changes partially address #3819
These changes are downstream of changes in (cylc-rose:#20)[https://github.com/cylc/cylc-rose/pull/20]. The PR attempts to add command line options similar to the following
rose suite-run
options:--opt-conf-key
--defines
--suite-defines
Checklist