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

Fix trailing slash in reg (workflow name) bug #4011

Merged
merged 2 commits into from
Jan 11, 2021

Conversation

MetRonnie
Copy link
Member

@MetRonnie MetRonnie commented Dec 22, 2020

These changes close #4010

It seems that passing a reg with a trailing slash to the mutations_kwargs dict was the reason for this bug. As such I've fixed it everywhere I found a mutations_kwargs dict.

Does this need a functional test?

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).
  • Does not need tests
  • Appropriate change log entry included.
  • No documentation update required.
  • No dependency changes.

@MetRonnie MetRonnie added bug Something is wrong :( small labels Dec 22, 2020
@MetRonnie MetRonnie added this to the cylc-8.0a3 milestone Dec 22, 2020
@MetRonnie MetRonnie self-assigned this Dec 22, 2020
@oliver-sanders
Copy link
Member

Did you get to the bottom of why the exact argument specified to cylc run mattered?

Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

I think this is harmless and should work for both REG/path uses, .. in paths now get expanded.

Not so worried about testing as the universal identifier work will centralise this.

@MetRonnie
Copy link
Member Author

MetRonnie commented Dec 22, 2020

Did you get to the bottom of why the exact argument specified to cylc run mattered?

I assumed it's something to do with how mutations are handled, seeing as removing the slash before the mutations_kwargs dict is set is what seems to fix it. (But I have no experience with the mutations stuff)

@hjoliver
Copy link
Member

hjoliver commented Dec 22, 2020

I'm not sure this is the right solution.

If the registered suite name is "foo", then "foo/" is not the correct name, so cylc stop foo/ should say SuiteStopped: foo/ is not running rather than magically convert foo/ to foo. (And vice versa).

If that's confusing because it's easier to accidentally register foo/ than some other wrong character, then we should disallow / at the end of a suite name.

@MetRonnie
Copy link
Member Author

The trouble with that is tab autocompletion adds a trailing slash to the directory name, so it would be an inconvenience to have to remove it every time.

@hjoliver
Copy link
Member

OK fair point. Approving on the basis that suite name often mirrors source dir name

@hjoliver hjoliver merged commit 8723426 into cylc:master Jan 11, 2021
@MetRonnie MetRonnie deleted the cylc-stop-bug branch January 11, 2021 21:46
@hjoliver hjoliver modified the milestones: cylc-8.0a3, cylc-8.0b0 Feb 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is wrong :( small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cylc stop doesn't work when reg ends with slash
3 participants