Skip to content

Escapes outside of variable substitution - #27

Merged
bradrydzewski merged 3 commits into
drone:masterfrom
jsturtevant:escape-issue
Mar 5, 2021
Merged

Escapes outside of variable substitution#27
bradrydzewski merged 3 commits into
drone:masterfrom
jsturtevant:escape-issue

Conversation

@jsturtevant

Copy link
Copy Markdown
Contributor

This demonstrates #26:

eval_test.go:211: \\something ${var=${default_var}}
eval_test.go:220: Want "\\\\something ${var=${default_var}}" expanded to "\\\\something foo", got "\\something foo" 

Once get confirmation on behavior expected can update.

@tboerger

Copy link
Copy Markdown
Contributor

Sounds valid for me, now code got to be fixed :)

@jsturtevant

Copy link
Copy Markdown
Contributor Author

I added a few more tests cases and updated so only the $$ gets escaped during normal scanning but all characters get scanned when inside a variable replace function. PTAL

@jsturtevant

Copy link
Copy Markdown
Contributor Author

@tboerger gentle ping. If there is a better approach, happy to adjust

@bradrydzewski

Copy link
Copy Markdown
Contributor

thanks @jsturtevant. I think this looks good. I always felt like we had to do some extra escaping in Drone (which uses this library) and it always felt off ... this bug probably explains why :) I think fixing this bug might break some existing Drone yaml files that are compensating for this escaping bug you identified. Perhaps this is a good time to bump this to a 2.0 go module version. Thoughts @tboerger ?

@jsturtevant

Copy link
Copy Markdown
Contributor Author

@tboerger @bradrydzewski any further thoughts here?

@tboerger

Copy link
Copy Markdown
Contributor

Bumping the version sounds valid to me. This could be added to Drone 2.0 in that case.

@jsturtevant

Copy link
Copy Markdown
Contributor Author

Is bumping the version something I need to do in this PR or is that a follow up? Would like to get this update in so we can unblock the capi scenario it is used in: kubernetes-sigs/cluster-api#4016

@bradrydzewski

Copy link
Copy Markdown
Contributor

hey James, apologies for not being more responsive. Yes, if you could help bump the version number as part of this pull request that would be great. I also think having more people involved and able to merge would be great, and would like to grant you maintainer access, if you are interested ... that way you can help push through time sensitive pull requests related to kubernetes.

@jsturtevant

Copy link
Copy Markdown
Contributor Author

@bradrydzewski I've updated the module version.

Regarding the maintainer access, I could help out. Do you have any docs on your release process or other things like that? Is there a communication channel you hang out in for drone (i.e. slack or something similiar)?

@bradrydzewski
bradrydzewski merged commit 490366e into drone:master Mar 5, 2021
@bradrydzewski

Copy link
Copy Markdown
Contributor

@jsturtevant thanks for the contribution. we are working on making Drone more of an open community so please bear with us; we do not have a lot of process or documentation in place yet. @tphoney any advice on what we can / should put in place here?

ecrupper pushed a commit to go-vela/server that referenced this pull request Apr 18, 2023
…cters causes failure (#793)

* fix(compiler): update envsubst to v2

Use the implementation made upstream in drone/envsubst#27 to fix escape sequence handling to prevent yaml parsing to fail due to invalid escape sequences.

Issue: go-vela/community#702

* test(compiler): handle escape sequences

* chore: use latest go-vela/types

---------

Co-authored-by: Jordan Brockopp <jordan.brockopp@target.com>
Co-authored-by: Jacob Floyd <cognifloyd@gmail.com>
Co-authored-by: dave vader <48764154+plyr4@users.noreply.github.com>
Co-authored-by: David May <49894298+wass3rw3rk@users.noreply.github.com>
Co-authored-by: NickHackman <Nick.Hackman@target.com>
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.

3 participants