[Airship-discuss] Divingbell overrides functionality regression - Assistance needed

[ANDERSON, CRAIG]

I was able to get it working, so the PS is green now. Swapping the arguments order for the calls made to `mergeOverwrite` fixed it to give the correct (overrides) dict precedence. The reason the arguments were "backwards" before was because mergo was very fussy about ordering and would core dump when we tried using merge with the "intuitive" argument order. Given that the test passed without encountering that error either, it seems we can conclude this is resolved in the current version of mergo.

The original problem must have been due to a change in the original `merge` function, which actually appears to be behaving the same as the `mergeOverwrite` function added in Helm after the mergo update. Therefore I believe the test would likely also pass on Helm v2.12.3 using the `merge` function as before but with swapped argument positions. However I think we should just do the Helm v2.13.0 upgrade anyway unless someone has an objection, or before someone proves my Helm v2.12.3 theory wrong :)

Roy, it may also be interesting to see if we have issues with `null` parameters in this version of Helm. If this works now without throwing nil pointer exceptions, we could just add this test into the gate script to make sure it doesn't show up again (I'm crossing my fingers).

Thanks,

-----Original Message-----
From: Roman Gorshunov [mailto:paye600 at gmail.com] 
Sent: Thursday, March 07, 2019 4:04 PM
To: ANDERSON, CRAIG <ca846m at att.com>; airship-discuss at lists.airshipit.org
Cc: TANG, ROY <rt7380 at att.com>
Subject: Re: Divingbell overrides functionality regression - Assistance needed

Good news:
Tests on gates have succeeded.
I have formatted a proper patch set which is now ready for review [0].
If applied, this patch will update Helm from v2.12.3 to v2.13.0, and replace `merge` with `mergeOverwrite` where appropriate in *daemonset_overrides.tpl files from helm-toolkit and ceph-osd. I'm actually not sure how to test if changes in *daemonset_overrides.tpl are correct.

Not so good news:
This didn't help to the divingbell overrides, and they are still not working.
Logs: https://urldefense.proofpoint.com/v2/url?u=http-3A__paste.openstack.org_show_747433_&d=DwIBaQ&c=LFYZ-o9_HUMeMTSQicvjIg&r=aimn2OylFgog_5_aS85wtQ&m=C3ro_V1fT7fdpF_d9veBkPBnYfsWSeCOKV8ajDB3C-8&s=AzlCjMLPTU89x1SC_X7C2jE78TYtRST2DUGPMmM0Jpc&e=

I'm stuck again.

[0] https://urldefense.proofpoint.com/v2/url?u=https-3A__review.openstack.org_-23_c_641656_5&d=DwIBaQ&c=LFYZ-o9_HUMeMTSQicvjIg&r=aimn2OylFgog_5_aS85wtQ&m=C3ro_V1fT7fdpF_d9veBkPBnYfsWSeCOKV8ajDB3C-8&s=_tTepiy_shl58qYy5kHFP0r3wx6-JHpiCYDEq5pxJh8&e=

-- Roman Gorshunov

On Thu, Mar 7, 2019 at 2:17 PM Roman Gorshunov <paye600 at gmail.com> wrote:
>
> Playing a little bit, I have just found out that using helm version
> v2.13.0-rc.1 and higher, AND `mergeOverwrite` allows to pass linting.
> Log: 
> https://urldefense.proofpoint.com/v2/url?u=http-3A__paste.openstack.or
> g_show_747402_&d=DwIBaQ&c=LFYZ-o9_HUMeMTSQicvjIg&r=aimn2OylFgog_5_aS85
> wtQ&m=C3ro_V1fT7fdpF_d9veBkPBnYfsWSeCOKV8ajDB3C-8&s=d-lYsZ5PXoRxgXFJCW
> dj84tcen9JqEuTvztcEEujT3I&e=
>
> I've submitted a test PS to see if it would pass standard voting
> gates: 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__review.openstack.
> org_-23_c_641656_&d=DwIBaQ&c=LFYZ-o9_HUMeMTSQicvjIg&r=aimn2OylFgog_5_a
> S85wtQ&m=C3ro_V1fT7fdpF_d9veBkPBnYfsWSeCOKV8ajDB3C-8&s=hSiLdJ1M9Hv0zlQ
> pbKg2gogxOuiMZ6jVdmRQaTdp1D0&e= The non-voting 
> `openstack-helm-infra-airship-divingbell` gate should still fail, as 
> it would use old `merge`.
>
> Waiting for gates to complete the work.
>
> -- Roman Gorshunov
>
> On Thu, Mar 7, 2019 at 1:55 PM Roman Gorshunov <paye600 at gmail.com> wrote:
> >
> > > Just to confirm, were you using Helm 1.12.x when you tried replacing `merge` with `mergOverwrite`?
> > I used helm 2.12.3.I confirm that mergeOverwrite does not work.
> >
> > Link with test: 
> > https://urldefense.proofpoint.com/v2/url?u=http-3A__paste.openstack.
> > org_show_747400_&d=DwIBaQ&c=LFYZ-o9_HUMeMTSQicvjIg&r=aimn2OylFgog_5_
> > aS85wtQ&m=C3ro_V1fT7fdpF_d9veBkPBnYfsWSeCOKV8ajDB3C-8&s=AwTIrAw_OBoL
> > 4c98ESbnyVIQzkeFPy5YFi9CFL8Ewak&e=
> > Details:
> > - line  4: helm client version v2.12.3 (this version is used in 
> > openstack-helm-infra by default)
> > - line 16: latest openstack-helm-infra (commit id
> > e836707ad070e910d6130eced066bc0e50481d2b)
> > - line 30: `helm lint` of helm-toolkit
> > - line 33: ./templates/utils/_daemonset_overrides.tpl uses `merge`
> > - line 40: replace `merge` with `mergeOverwrite`
> > - line 41: confirm replacement
> > - line 50: linting fails with error `function "mergeOverwrite" not 
> > defined`
> >
> > Thank you for your help, Craig.
> >
> > -- Roman Gorshunov