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

Use greedy regex for $(HOST: ) #134

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zyga
Copy link
Contributor

@zyga zyga commented Sep 8, 2021

I cannot think of the reason the original code used non-greedy
matching, given that YAML allows us to control the text of the
string quite strongly, and there is nothing we want to parse after the
closing parenthesis.

This fixes variables which use ) inside the shell command.

Signed-off-by: Zygmunt Krynicki me@zygoon.pl

I cannot think of the reason the original code used non-greedy
matching, given that YAML allows us to control the text of the
string quite strongly, and there is nothing we want to parse after the
closing parenthesis.

This fixes variables which use ) inside the shell command.

Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
@mvo5
Copy link
Contributor

mvo5 commented Sep 13, 2021

Some context from irc:

<mvo> zyga-mbp: I quickly looked at the PR, if you could add an example how you noticed it and a "before/after" the change that would be lovely, then I can write a test for you
<zyga-mbp> yeah, I can even write the test but I didn't get to it yet, it's something I bumped into yesterday
<zyga-mbp> the example was $(HOST: $(echo example))
<zyga-mbp> or maybe
<zyga-mbp> the example was $(HOST: $(echo example)-foo)
<zyga-mbp> anyway, I'll update it with unit tests
<mvo> zyga-mbp: cool, that is all I need
<zyga-mbp> the real code was in libzt test suite, I've updated it to support local snap store proxy for caching
<zyga-mbp> and I've used quite a bit of HOST variables for that
<zyga-mbp> I can paste the real thing but the point is that the old spread stopped at the first right ")"
<zyga-mbp> so $(HOST: ...) could never have ) insdie
<zyga-mbp> *inside
``

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.

None yet

2 participants