search

unbounce / iidy

iidy (Is it done yet?) -- CloudFormation with Confidence
MIT License
52 stars 7 forks source link

implement UsePreviousParameterValues per #170 #207

Closed tavisrudd closed 4 years ago

tavisrudd commented 5 years ago

Implements #170. I'll follow up with tests on this branch. @dannosaur can you give it a spin in the meantime?

dannosaur commented 5 years ago

@tavisrudd you're a legend! I'm actually in a position where I'm just about to implement CI for a project built using iidy, so I'll give this a thorough rundown next week on a test stack and let you know how things go.

dannosaur commented 5 years ago

It doesn't work. From what I can see, it's because with for (const paramName in ..), paramName is actually an index rather than the parameter's name. It seems that this needs to be changed to use array.forEach((paramName, index) => { .. });.

Furthermore, input.Parameters isn't keyed by the parameter's name, but rather is just a simple array of objects. One must inspect each object individually, checking whether ParameterKey == paramName before doing the override.

I have managed to fix this in e303f3c, and it appears to work as expected.

tavisrudd commented 5 years ago

Teaches me to quickly fire stuff off late in the afternoon with not enough thought and no testing :) I’ll try your fix later today. Thanks

On Mon, Oct 28, 2019 at 8:52 AM Dan Jones notifications@github.com wrote:

It doesn't work. From what I can see, it's because with for (const paramName in ..), paramName is actually an index rather than the parameter's name. It seems that this needs to be changed to use array.forEach((paramName, index) => { .. });.

Furthermore, input.Parameters isn't keyed by the parameter's name, but rather is just a simple array of objects. One must inspect each object individually, checking whether ParameterKey == paramName before doing the override.

I have managed to fix this in e303f3c https://github.com/unbounce/iidy/commit/e303f3c9997ae64d1c2c22fc0d4497f639e23725, and it appears to work as expected.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/unbounce/iidy/pull/207?email_source=notifications&email_token=AABZ2VWFJCHFOIWMH5SPKOTQQ4DELA5CNFSM4JFKLYE2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECNMAIY#issuecomment-547012643, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABZ2VXGJPHFFMYPJNPONH3QQ4DELANCNFSM4JFKLYEQ .

tavisrudd commented 5 years ago

This looks good btw, I'm just holding off until I add some tests.