Skip to content

Tidup sidekiq queues in case they're multiline

Craig Miskell requested to merge tidy-up-sidekiq-queues into master

What does this MR do?

Makes the sidekiq container a little more robust against the sort of inputs that might come through helm unintentionally, in this case multi-block lines with

Related issues

These issues are where we found this edge case:

Discussion

To be clear:

  1. I'm still not entirely sure where the extra single quote came from in the above issue, but I'm not sure that ultimately matters; the trailing newline was the trigger.
  2. Yes, technically the problem here lies in the usage of the helmchart (not even a bug in the helmchart itself, unless there's some way to enforce non-multi-line strings), but it's entirely too easy to do and the results were a bit subtle and hard to spot.

This MR is belt'n'braces to protect users from themselves, and as such it's more of a politeness (and not mandatory) than a fatal flaw that absolutely must be fixed. So I'm open to further discussion.

For validation:

  • what should be passed to SIDEKIQ_QUEUES is a comma-separated list, e.g. "foo,bar,baz".
  • what was passed in the problem case was "foo,bar,baz\n" (not sure if it was an escaped \n or a literal, nor how to reasonably tell)
  • this should also handle embedded multi-lines, e.g. "foo,bar\n,baz,bug\n" and so on

There are still a plethora of ways to misconfigure it, e.g. "foo,bar\nbaz" with no trailing comma on a non-last line, would come out as "foo,barbaz", but that is harder to fix but also should be a little more obvious if debugging a failed deployment. But the fact this remains so brittle may suggest that this is a losing battle and we shouldn't even do this MR. Again, open to discussion.

Checklist

See Definition of done.

For anything in this list which will not be completed, please provide a reason in the MR discussion

Required

  • Merge Request Title, and Description are up to date, accurate, and descriptive
  • MR targeting the appropriate branch
  • MR has a green pipeline on GitLab.com

Expected (please provide an explanation if not completing)

  • Test plan indicating conditions for success has been posted and passes
  • Documentation created/updated
  • Integration tests added to GitLab QA
  • The impact any change in container size has should be evaluated
Edited by Craig Miskell

Merge request reports