Delete opts after post install #312

wxtim · 2024-04-16T12:06:14Z

Closes cylc/cylc-flow#5968 alongside a cylc/cylc-flow#6068.

Additionally closes cylc/cylc-flow#6058, which was caused by an earlier incomplete solution cylc/cylc-flow#5996

Rose variables should be removed from the options object after post install plugin to prevent them being used by any downstream Cylc Scripts.

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 · 2024-04-16T12:06:50Z

tests/unit/test_functional_post_install.py

@@ -353,10 +353,3 @@ def broken():
    (tmp_path / 'rose-suite.conf').touch()
    with pytest.raises(FileNotFoundError):
        rose_fileinstall(srcdir=tmp_path, rundir=tmp_path)
-
-
-def test_cylc_no_rose(tmp_path):


Precisely duplicated test_no_rose_suite_conf_in_devdir at line 60.

…ts objects.

Removed the runN from the tests

setup.cfg

cylc/rose/entry_points.py

tests/conftest.py

tests/functional/test_reinstall.py

oliver-sanders · 2024-04-16T16:29:12Z

tests/conftest.py

+        if not args or not args.get('workflow_name', ''):
+            id_ = test_workflow_name()
+            args = {'workflow_name': id_}
+
+        args.update({'no_run_name': True})
        options = Options(install_gop(), args)()
        output = SimpleNamespace()
+        output.id = args['workflow_name']


FYI, this is going to conflict when merged into master because all the output.ret, output.exc, output.out, etc stuff was stripped in favour of native pytest functionalities.

I think what you've added here mirrors the behaviour on master so should be easy to reconcile.

cylc-rose/tests/conftest.py

Lines 175 to 205 in ed485a8

def _cylc_install_cli(test_dir):

"""Access the install CLI"""

async def _inner(srcpath, workflow_name=None, opts=None):

"""Install a workflow.

Args:

srcpath:

The workflow to install

workflow_name:

The workflow ID prefix to install this workflow as.

If you leave this blank, it will use the module/function's

test directory as appropriate.

opts:

Dictionary of arguments for cylc install.

"""

nonlocal test_dir

if not workflow_name:

workflow_name = str(

(test_dir / str(uuid4())[:4]).relative_to(CYLC_RUN_DIR)

)

options = Options(

install_gop(), opts or {}

)(workflow_name=workflow_name)

if not options.workflow_name:

options.workflow_name = workflow_name

if not opts or not opts.get('no_run_name', ''):

options.no_run_name = True

return await cylc_install(options, str(srcpath))

return _inner

cylc/rose/entry_points.py

Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>

…/cylc-rose into fix.delete_opts_after_post-install * 'fix.delete_opts_after_post-install' of github.com:wxtim/cylc-rose: Update setup.cfg

oliver-sanders · 2024-04-25T11:43:20Z

@wxtim, tests are still failing I'm afraid.

wxtim · 2024-04-25T14:35:40Z

@wxtim, tests are still failing I'm afraid.

I think that I've fixed it.

I don't like the functional testing I've built, but because I don't anticipate there ever being a need for much functional testing I don't think it's worth building anything better.

Cylc Rose should never have needed these tests, and they only demonstrate that it doesn't have functionality it wasn't meant to have.

cylc/rose/entry_points.py

tests/conftest.py

MetRonnie · 2024-04-26T10:28:14Z

I think that I've fixed it.

I don't like the functional testing I've built, but because I don't anticipate there ever being a need for much functional testing I don't think it's worth building anything better.

Cylc Rose should never have needed these tests, and they only demonstrate that it doesn't have functionality it wasn't meant to have.

You seem to have removed the tests you added. At the moment there are no tests added in this PR

tests/conftest.py

…ter_post-install * upstream/1.3.x: Fix changelog

MetRonnies test fix

MetRonnie

Couple of small suggestions

cylc/rose/entry_points.py

tests/conftest.py

Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>

wxtim added 2 commits April 16, 2024 12:56

delete unwanted opts after post install step.

1f3dcc4

Removed test because it duplicated test_no_rose_suite_conf_in_devdir

d212068

wxtim marked this pull request as draft April 16, 2024 12:06

wxtim commented Apr 16, 2024

View reviewed changes

added test to confim that post instlall removes Rose settings from op…

2278f31

…ts objects.

wxtim force-pushed the fix.delete_opts_after_post-install branch from 7773d76 to 2278f31 Compare April 16, 2024 13:06

wxtim self-assigned this Apr 16, 2024

wxtim changed the base branch from master to 1.3.x April 16, 2024 13:06

wxtim changed the title ~~Fix.delete opts after post install~~ Delete opts after post install Apr 16, 2024

lower pin cylc version for this fix

06b5ab7

wxtim mentioned this pull request Apr 16, 2024

Play should have no cylc rose options cylc/cylc-flow#6068

Merged

10 tasks

Added a test for Cylc Reload overriding existing tvars correctly

f96d468

Removed the runN from the tests

wxtim commented Apr 16, 2024

View reviewed changes

setup.cfg Outdated Show resolved Hide resolved

wxtim requested a review from oliver-sanders April 16, 2024 14:14

wxtim marked this pull request as ready for review April 16, 2024 14:14

wxtim requested a review from MetRonnie April 16, 2024 14:15

wxtim modified the milestones: 1.3.x, 1.4.0 Apr 16, 2024

oliver-sanders added the bug Something isn't working label Apr 16, 2024

MetRonnie reviewed Apr 16, 2024

View reviewed changes

cylc/rose/entry_points.py Show resolved Hide resolved

tests/conftest.py Outdated Show resolved Hide resolved

tests/functional/test_reinstall.py Outdated Show resolved Hide resolved

oliver-sanders modified the milestones: 1.4.0, 1.3.x Apr 16, 2024

oliver-sanders reviewed Apr 16, 2024

View reviewed changes

cylc/rose/entry_points.py Outdated Show resolved Hide resolved

Update tests/conftest.py

2dd84cc

Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>

Merge branch 'fix.delete_opts_after_post-install' of github.com:wxtim…

c87270b

…/cylc-rose into fix.delete_opts_after_post-install * 'fix.delete_opts_after_post-install' of github.com:wxtim/cylc-rose: Update setup.cfg

wxtim requested a review from oliver-sanders April 25, 2024 08:25

wxtim marked this pull request as ready for review April 25, 2024 08:25

wxtim marked this pull request as draft April 25, 2024 13:37

wxtim force-pushed the fix.delete_opts_after_post-install branch from 1fcb22c to 032ba65 Compare April 25, 2024 14:40

fix functional tests

9df1d59

wxtim force-pushed the fix.delete_opts_after_post-install branch from 032ba65 to 9df1d59 Compare April 25, 2024 14:45

wxtim marked this pull request as ready for review April 25, 2024 14:47

MetRonnie reviewed Apr 26, 2024

View reviewed changes

cylc/rose/entry_points.py Outdated Show resolved Hide resolved

tests/conftest.py Outdated Show resolved Hide resolved

MetRonnie reviewed Apr 26, 2024

View reviewed changes

tests/conftest.py Outdated Show resolved Hide resolved

Update tests/conftest.py

c7c9680

wxtim marked this pull request as draft April 26, 2024 12:19

wxtim added 2 commits April 26, 2024 16:01

Update cylc/rose/entry_points.py

be04c4b

Merge remote-tracking branch 'upstream/1.3.x' into fix.delete_opts_af…

a6f51c3

…ter_post-install * upstream/1.3.x: Fix changelog

wxtim marked this pull request as ready for review April 30, 2024 10:05

wxtim requested a review from MetRonnie April 30, 2024 10:06

Fix testing

101e766

MetRonnies test fix

wxtim force-pushed the fix.delete_opts_after_post-install branch from 773e473 to 101e766 Compare April 30, 2024 10:17

MetRonnie approved these changes Apr 30, 2024

View reviewed changes

cylc/rose/entry_points.py Outdated Show resolved Hide resolved

tests/conftest.py Outdated Show resolved Hide resolved

wxtim and others added 2 commits April 30, 2024 11:44

Apply suggestions from code review

cb8b102

Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>

correct mistake

8a13bcf

MetRonnie approved these changes Apr 30, 2024

View reviewed changes

oliver-sanders approved these changes May 1, 2024

View reviewed changes

oliver-sanders merged commit 5073f5b into cylc:1.3.x May 1, 2024
4 of 5 checks passed

MetRonnie mentioned this pull request May 1, 2024

Changed behaviour in handling template variables passed via cli cylc/cylc-flow#6058

Closed

oliver-sanders modified the milestones: 1.3.x, 1.3.4 May 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delete opts after post install #312

Delete opts after post install #312

wxtim commented Apr 16, 2024 •

edited by MetRonnie

Loading

wxtim Apr 16, 2024

oliver-sanders Apr 16, 2024

oliver-sanders commented Apr 25, 2024

wxtim commented Apr 25, 2024 •

edited

Loading

MetRonnie commented Apr 26, 2024

MetRonnie left a comment

	def _cylc_install_cli(test_dir):
	"""Access the install CLI"""
	async def _inner(srcpath, workflow_name=None, opts=None):
	"""Install a workflow.

	Args:
	srcpath:
	The workflow to install
	workflow_name:
	The workflow ID prefix to install this workflow as.

	If you leave this blank, it will use the module/function's
	test directory as appropriate.
	opts:
	Dictionary of arguments for cylc install.

	"""
	nonlocal test_dir
	if not workflow_name:
	workflow_name = str(
	(test_dir / str(uuid4())[:4]).relative_to(CYLC_RUN_DIR)
	)
	options = Options(
	install_gop(), opts or {}
	)(workflow_name=workflow_name)
	if not options.workflow_name:
	options.workflow_name = workflow_name
	if not opts or not opts.get('no_run_name', ''):
	options.no_run_name = True
	return await cylc_install(options, str(srcpath))
	return _inner

Delete opts after post install #312

Delete opts after post install #312

Conversation

wxtim commented Apr 16, 2024 • edited by MetRonnie Loading

wxtim Apr 16, 2024

Choose a reason for hiding this comment

oliver-sanders Apr 16, 2024

Choose a reason for hiding this comment

oliver-sanders commented Apr 25, 2024

wxtim commented Apr 25, 2024 • edited Loading

MetRonnie commented Apr 26, 2024

MetRonnie left a comment

Choose a reason for hiding this comment

wxtim commented Apr 16, 2024 •

edited by MetRonnie

Loading

wxtim commented Apr 25, 2024 •

edited

Loading