puppetlabs-docker icon indicating copy to clipboard operation
puppetlabs-docker copied to clipboard

Improve param_changed detection (#881)

Open bmagistro opened this issue 3 years ago • 3 comments

This attempts to fix the issues identified in #881 and while it does address the identified issues in our scenario, I have some concerns about the approach/sanity of these checks related to the concerns called out by @mastermind2k in https://github.com/puppetlabs/puppetlabs-docker/issues/781#issuecomment-1242956951 .

In this case, the comparison for the mounts + volumes does not preserve the source/destination paring so while comparing the list of source and destination mounts might be correct, the pairing could still be wrong ie source1:dest2 when it should be source1:dest1 and wound not trigger the param_changed event that would be desired. When talking about configured volumes, It also does not take into account the mount type such as volume, bind, etc (not sure if this one should as it could be multiple types and still be a valid match in my opinion).

It also seems like this function may break when called on a windows host. All of the start, stop, create, etc calls wrap the command with run_with_powershell while the detect_changes does not.

I would appreciate hearing from @adrianiurca who authored most of this function as I had to guess at a couple things so if additional light can be shed on what it should be accomplishing I can try to make some updates.

bmagistro avatar Dec 14 '22 03:12 bmagistro

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Dec 14 '22 03:12 CLAassistant

docker_params_changed is a function

that may have no external impact to Forge modules.

This module is declared in 6 of 580 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.

Hi @bmagistro, thanks for taking the time to work on this issue. Unfortunately, @adrianiurca is not working in Puppet anymore so we don't think it's going to be possible to get more insight around this update than what we already have.

LukasAud avatar Jan 27 '23 16:01 LukasAud