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

Avoid %% escape in xtrigger func args (7.8.x). #3257

Merged
merged 5 commits into from
Aug 2, 2019

Conversation

hjoliver
Copy link
Member

@hjoliver hjoliver commented Jul 30, 2019

These changes close #3256

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).
  • Includes an appropriate entry in the release change log CHANGES.md.
  • (7.8.x branch) I have updated the documentation in this PR branch.

@hjoliver hjoliver added the bug Something is wrong :( label Jul 30, 2019
@hjoliver hjoliver added this to the next-release milestone Jul 30, 2019
@hjoliver hjoliver self-assigned this Jul 30, 2019
@hjoliver
Copy link
Member Author

hjoliver commented Jul 30, 2019

Test using the built-in echo xtrigger, which just echoes its function args (run in debug mode to see the result in the server log):

# workflow "test3256"

[scheduling]
  [[xtriggers]]
     a = echo(%(point)s, '%%(asdf)s')
  [[dependencies]]
     graph = "@a => foo"
[runtime]
  [[foo]]
     script = sleep 10

In the function args, %(point)s is a valid string template for cycle point; %%(asdf)s is not a template because % is escaped, so it should be passed in as a string literal.

On current 7.8.x and master branches (reproduces the bug):

[oliverh@niwa-1007885 cylc-flow]$ cylc val test3256
Illegal template in xtrigger a: asdf

On this branch, validation and run:

[oliverh@niwa-1007885 cylc-flow]$ cylc val test3256
Valid for cylc-7.8.3-13-g32f85

[oliverh@niwa-1007885 cylc-flow]$ cylc run --no-detach --debug test3256
            ._.                                                       
            | |            The Cylc Suite Engine [7.8.3-13-g32f85]    
._____._. ._| |_____.           Copyright (C) 2008-2019 NIWA          
| .___| | | | | .___|   & British Crown (Met Office) & Contributors.  
| !___| !_! | | !___.  _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
!_____!___. |_!_____!  This program comes with ABSOLUTELY NO WARRANTY;
      .___! |          see `cylc warranty`.  It is free software, you 
      !_____!           are welcome to redistribute it under certain  
2019-07-30T13:51:23+12:00 DEBUG - creating suite run directory: /home/oliverh/cylc-run/test3256
...(SNIP)...
2019-07-30T13:51:24+12:00 DEBUG - [xtrigger-func cmd] cylc-function-run echo '["1", "%(asdf)s"]' '{}' /home/oliverh/suites/foo
        [xtrigger-func ret_code] 0
        [xtrigger-func out] [false, {}]
        [xtrigger-func err]
        echo: ARGS: (u'1', u'%(asdf)s')  <-----------------------BINGO!
        echo: KWARGS: {}
2019-07-30T13:51:24+12:00 DEBUG - echo(1, %(asdf)s): returned {}

@hjoliver hjoliver changed the title Avoid %% escape in xtrigger function args. Avoid %% escape in xtrigger func args (7.8.x). Jul 30, 2019
RE_STR_TMPL = re.compile(r'%\(([\w]+)\)s')
# Extract 'foo' from string templates '%(foo)s', avoiding '%%' escaping
# ('%%(foo)s` is not a string template).
RE_STR_TMPL = re.compile(r'(?<!%)%\(([\w]+)\)s')
Copy link
Member Author

@hjoliver hjoliver Jul 30, 2019

Choose a reason for hiding this comment

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

Zero-width negative look behind assertion, innit 😁

@trwhitcomb
Copy link
Collaborator

You beat me to it! I used an exclusion class (i.e. r[^%]%\() instead of any fancy lookbehinds though.

@hjoliver
Copy link
Member Author

You beat me to it! I used an exclusion class (i.e. r[^%]%\() instead of any fancy lookbehinds though.

Sorry about that! However, I don't think the exclusion class works because it is not zero width. i.e. it would match x%(blah)s but not plain old %(blah)s. (And adding a "zero or one of" qualifier doesn't fix the problem because the other % gets in the way then?)

@trwhitcomb
Copy link
Collaborator

trwhitcomb commented Jul 30, 2019

I think you're right, but because the exclusion isn't part of the capturing group, even if it did match x%(blah)s it should still only pick up blah which is what this code is looking for.

I don't have an issue with the zero width lookaround, but I will confess that I didn't recognize it immediately. I think the lookaround will also permit two of these format strings to be concatenated, which would fail to match using the exclusive character class.

@kinow
Copy link
Member

kinow commented Jul 30, 2019

Maybe we should include a few unit tests covering the cases discussed above so far ☝️ ? 😬

@hjoliver
Copy link
Member Author

Maybe we should include a few unit tests covering the tests above point_up ? grimacing

Yes, I've left this as a WIP/Draft pending time to think about tests.

@kinow
Copy link
Member

kinow commented Jul 30, 2019

Oh, sorry, didn't look at the checklist.

@trwhitcomb
Copy link
Collaborator

Now I'm very glad my fix got scooped by @hjoliver!

@hjoliver hjoliver marked this pull request as ready for review August 2, 2019 02:31
@hjoliver hjoliver requested a review from kinow August 2, 2019 02:31
@hjoliver
Copy link
Member Author

hjoliver commented Aug 2, 2019

God damn it. Style fail.

@hjoliver
Copy link
Member Author

hjoliver commented Aug 2, 2019

@trwhitcomb - I've added you as a "collaborator" on this repository so I can assign you to review this PR. You should get an invite from GitHub.

@cylc cylc deleted a comment Aug 2, 2019
@matthewrmshin
Copy link
Contributor

Test failures are related to #3244 - Travis CI hostname issue.

@kinow kinow merged commit c61efc6 into cylc:7.8.x Aug 2, 2019
oliver-sanders added a commit to oliver-sanders/cylc-flow that referenced this pull request Sep 14, 2023
* Move the updater into another process.
  This removes the limitation that caused Tui to crash with active workflows.
  Closes cylc#3257
* Add multi-workflow capability.
  Closes cylc#3464
* Add visual regression testing framework.
  Closes cylc#3530
@oliver-sanders oliver-sanders mentioned this pull request Sep 14, 2023
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is wrong :(
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants