search

buildkite / feedback

Got feedback? Please let us know!
https://buildkite.com
25 stars 24 forks source link

Unexpected `\r` at the end of line #279

Closed raynix closed 5 years ago

raynix commented 6 years ago

When using something like export MYLISTS=$(buildkite-agent meta-data get "mylist") where mylist has multiple lines from the form input, I got a \r at the end of each line. Is this expected? I though it should be \n.

toolmantim commented 6 years ago

@raynix is this on Windows or Linux? We might have a bug for Windows here.

raynix commented 6 years ago

@toolmantim sorry I didn't get notified for your reply... We run agents on containers based on Debian stretch docker image.

toolmantim commented 6 years ago

@raynix sorry, no worries. Ah, I think I understand the context now… was this a click to unblock w/ fields in the web UI, where they entered multiple lines? Do you know what browser they were using? Was it Windows, Mac or Linux?

raynix commented 6 years ago

Yes this is regarding the text field of a block command. I used Chrome on OS X @toolmantim

toolmantim commented 6 years ago

Thanks @raynix — that's all we need to investigate.

ticky commented 6 years ago

Okay, I’ve finally figured out why and where this is going on.

jQuery is in charge of submitting that form, and it (non-optionally?) “normalises” form value newlines to Windows-style cr-lf: https://github.com/jquery/jquery/blob/2.1.1/src/serialize.js#L82-L107

toolmantim commented 6 years ago

Ah! That'd be it @ticky. Looks like something we might need to fix on the backend then.

raynix commented 6 years ago

@ticky nice find! Shame shame jQuery :laughing:

ticky commented 6 years ago

@toolmantim I’m not sure there’s a “right” way to fix it on the backend. The “right” way might actually be for the agent to normalise (optionally) to the current platform’s native line endings (\r\n on Windows, \n on Unix, \r on Mac OS 9 and ea– can Go build on Mac OS 9??? 🤔).

Alternately, I did consider moving this page away from jQuery as it’s already constructed within React - then we could at least confidently say “what you put in is exactly what you get out” 🤦🏻‍♀️

ticky commented 6 years ago

An alternative which came to mind over lunch is… we could patch our vendored jQuery to not do this. 😂

ticky commented 6 years ago

Okay, I believe I have a viable patch - the question is what’s the right move for this. Should we normalise to \n, or not normalise at all?

toolmantim commented 6 years ago

I’d suggest normalizing to \n

ticky commented 6 years ago

The trouble with that is it potentially breaks things for Windows users, and I don’t know what the suggestion is for them to work around it?

toolmantim commented 6 years ago

I’d say normalize it to \n and document it, because at least it’s consistent between the people submitting fields in unblock steps, and you can write a script against the one format?

ticky commented 6 years ago

Okay I’ve updated the PRs to do so!

raynix commented 6 years ago

Consistency FTW!

On Oct 12, 2017 09:36, "Jessica Stokes" notifications@github.com wrote:

Okay I’ve updated the PRs to do so!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/buildkite/feedback/issues/279#issuecomment-335968772, or mute the thread https://github.com/notifications/unsubscribe-auth/ABifSYCHsnYWOs19xKfxnW0EuVkVKUBLks5srUMKgaJpZM4PelJZ .

keithpitt commented 5 years ago

(whoops, we forgot to close this issue) but we rolled out a fix last year for this! 🚀