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

Prevent cylc-run of creating directories for invalid suites #3409

Merged
merged 1 commit into from
Oct 17, 2019

Conversation

kinow
Copy link
Member

@kinow kinow commented Oct 13, 2019

These changes close #3097

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

@kinow kinow self-assigned this Oct 13, 2019
@kinow kinow added this to the cylc-8.0a2 milestone Oct 13, 2019

if not os.path.exists(suite_srv_dir):
sys.stderr.write(f'suite service directory not found '
f'at: {suite_srv_dir}\n')
Copy link
Member Author

Choose a reason for hiding this comment

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

Shamelessly copied this approach from cylc-cat-log.

@cylc cylc deleted a comment Oct 13, 2019
@kinow
Copy link
Member Author

kinow commented Oct 13, 2019

New behaviour:

$ cylc run mp3
suite service directory not found at: /home/kinow/cylc-run/mp3/.service

Tested cylc-register and it appears to be working fine. I think the code changed is not used by cylc-register.

@kinow kinow marked this pull request as ready for review October 13, 2019 11:21
@hjoliver
Copy link
Member

One test failed, probably real, or else a huge coincidence!

./flakytests/registration/02-on-the-fly.t              (Wstat: 0 Tests: 9 Failed: 3)

@kinow
Copy link
Member Author

kinow commented Oct 14, 2019

One test failed, probably real, or else a huge coincidence!

./flakytests/registration/02-on-the-fly.t              (Wstat: 0 Tests: 9 Failed: 3)

Kicked Travis as it was one of the flakytests 🤞

@hjoliver
Copy link
Member

I predict it'll fail again 😬 :

$ ./etc/bin/run-functional-tests.sh -v flakytests/registration/02-on-the-fly.t 

ok 1 - 02-on-the-fly-pwd-run
ok 2 - 02-on-the-fly-pwd-run.stdout-contains-ok
ok 3 - 02-on-the-fly-pwd-stop
not ok 4 - 02-on-the-fly-cylc-run-dir-run
not ok 5 - 02-on-the-fly-cylc-run-dir-run.stdout-contains-ok
not ok 6 - 02-on-the-fly-cylc-run-dir-stop
ok 7 - 02-on-the-fly-cylc-run-dir-2-validate
ok 8 - 02-on-the-fly-cylc-run-dir-2-validate.stdout-contains-ok
ok 9 - 02-on-the-fly-cylc-run-dir-2-validate.stderr-contains-ok

    stdout and stderr stored in: /tmp/oliverh/cylctb-20191014T090638Z/registration/02-on-the-fly
Failed 3/9 subtests 
[22:06:53]

Test Summary Report
-------------------
flakytests/registration/02-on-the-fly.t (Wstat: 0 Tests: 9 Failed: 3)
  Failed tests:  4-6
Files=1, Tests=9, 15 wallclock secs ( 0.02 usr  0.00 sys +  2.04 cusr  0.40 csys =  2.46 CPU)
Result: FAIL

@kinow
Copy link
Member Author

kinow commented Oct 14, 2019

Was re-creating my virtual env to run it, but you beat me to it. Let me take a look at the code. Thanks!

@kinow
Copy link
Member Author

kinow commented Oct 14, 2019

Take 2. The code was checking for ~/cylc-run/blabla/.service, which prevented the on-the-fly registration. I updated the code to now use get_suite_run_dir, and then test for ~/cylc-run/blabla.

Wondering if this is good enough, or if we must prevent the cylc run of running if:

  • no suite.rc or
  • no .service dir as before ?

@cylc cylc deleted a comment Oct 14, 2019
Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

Seems to have the desired behaviour, for normal run and on-the-fly reg. 👍

@oliver-sanders oliver-sanders merged commit f902059 into cylc:master Oct 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Avoid creating empty suite directories when running suites that do not exist
3 participants