docker_run_flags: Shellescape any provided values

[neomilium]

When passing an environment variable with special characters, it can broke the script or produce unexpected behavior. I encountered two of them:

• If a $ is in environment variable, its interpolated as a variable
• If a " is in environment variable, it broke the end of string, so the script

This PR contains a commit to escape any value that will be pushed in the docker start script.

[puppet-community-rangefinder[bot]]

docker_run_flags is a function

Breaking changes to this file WILL impact these 1 modules (exact match):

• glorpen-g_docker

Breaking changes to this file MAY impact these 6 modules (near match):

• puppetlabs-docker
• garethr-docker
• cristifalcas-docker
• hackerhappyhour-docker
• memoussati-docker
• mvasilenko-docker

This module is declared in 6 of 579 indexed public Puppetfiles.

---

These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

[CLAassistant]

All committers have signed the CLA.

[chelnak]

Hey 👋

Can you set this PR to ready for review?

thanks!

[smortex]

I am wondering if the CI failures are relevant… Like volume-1:C:\volume_1 being escaped as volume-1:C:\\volume_1… Not sure about how to check this…

[neomilium]

@chelnak Test added by @smortex, PR is now ready.

[chelnak]

@smortex - i'll take a closer look at this tomorrow. Seems odd that all of the failures here are for Windows and they don't seem to be on the nightly tests either..

[smortex]

@neomilium @chelnak I added a commit that seems to help… but escaping on windows is total nonsense and I am sure of nothing :scream:

Some windows node pass, the one failing seems to fail earlier than the code added / changed here and the failures might be the result of some external service not being reliable… maybe?

[smortex]

Last CI failures seems to be caused by inaccessible windows test nodes. I think this is ready for review.

[chelnak]

Failed jobs have been restarted. If everything looks ok I’d like to get this in to the next release.

[smortex]

@neomilium have you had a chance to see if the issue you originally tried to fix with this PR is indeed fixed with all these changes?

[canihavethisone]

@smortex @chelnak @bastelfreak - why has stdlib been raised to min v8.2.0 in the pull request? That is very high and will have breaking impact when combined with a number of other available modules

[smortex]

@smortex @chelnak @bastelfreak - why has stdlib been raised to min v8.2.0 in the pull request? That is very high and will have breaking impact when combined with a number of other available modules

Because we had to add escape functions for windows in the stdlib: see this discussion above and https://github.com/puppetlabs/puppetlabs-stdlib/pull/1235.

[chelnak]

@smortex @bastelfreak How do you both feel about moving forward with this PR?

Pending test failures which I think may be transient, I think it is good.

Though I do wonder if it is a breaking change due to the higher requirements of stdlib.

What are your thoughts?

[chelnak]

@smortex

I've chatted with @canihavethisone around some of the concerns raised in the comment above.

I personally think stdlib is the right place for the new escape commands however I want to get more of a feel for the potential impact on other users of the module.

This is also an issue that we will probably hit again with other modules. In order to take advantage of newer features in stdlib, the minimum version number will need to be raised.

[canihavethisone]

My concern is that the major fast forward of the stdlib dependency may have significant impact for users of this module in deployments alongside other modules that haven't caught up yet. I recon a good metric would be how many popular modules still have stdlib pinned lower than v.8x, and weigh that against the urgency of this change or if it can be merged when 8x is more commonly scoped. I understand there will be diversity of opinions but think its worth considering.

[neomilium]

I recon a good metric would be how many popular modules still have stdlib pinned lower than v.8x, and weigh that against the urgency of this change or if it can be merged when 8x is more commonly scoped.

How to define "popular modules"?

[canihavethisone]

@chelnak @smortex Hi, im not concerned about people blindly updating, I'm concerned that we will lock out users who use this module alongside numerous others. Locking them out will deny them of new features and bugfixes until their other dependencies support stdlib v8. Bottom line IMO is that there is a huge difference between setting the upper limit to v8 or the lower requirement to v8. Its also a cost v benefit consideration at this moment in v8's youth.

[binford2k]

Here's a list of all the Forge modules that pin their stdlib dependencies to <8.x, sorted by download count (a weak correlation to popularity.) Notice that many of them are deprecated.

https://docs.google.com/spreadsheets/d/16QT1wd5CmwJL9TbKe4GZgpPi3W4nXHo7Q4fD5aK8bRM/edit#gid=223629225

[LukasAud]

Hi @neomilium. After careful evaluation of your PR, I'm happy to inform you that we are ready to go ahead and merge it. Thanks for your contribution!

We also noted that there are some concerns about this PR, such as the one raised by @canihavethisone. We have considered the potential impact of these changes and we have decided to still go forward with it. While it is true that there may be some compatibility issues with a few non-supported modules, we also have noted that this change may be important for one of our current security enhancement projects and, as such, either this or something very similar was bound to be implemented sooner or later. We are sorry if this causes any inconvenience.

[LukasAud]

This PR will be temporarily reverted until a release can be cut. Then it will be reinstated. This process is done to follow with standard best practices.

[LukasAud]

Hey @neomilium. We are sorry for this inconvenience but we had to revert your PR in order to follow best practices when releasing a new version of our module, by making a minor release prior to a backwards incompatible one. We could re-implement it but I'm afraid it would appear under our name. Could you reopen your branch under a new PR so that we can merge and give proper credit to all contributors?

[kenyon]

Rebasing or cherry-picking the commits would maintain the original commit authors.