Should an exception be thrown for duplicate redis keys?

[shellandbull]

Hello peeps,

I'm working on a project that makes use of this library in our CI. We basically trigger a deployment every time a build passes successfully.

Given the current design of this project - and its defaults - retrying a successful build yields an error since the deploy script will output the same manifest, causing the following:

|    - Value already exists for key: zapnito-web:index:288d60899ac3cc175fcd2bbb758dc3de
|
+- didFail
Value already exists for key: zapnito-web:index:288d60899ac3cc175fcd2bbb758dc3de
undefined|
Pipeline aborted

I don't think retrying a successful build - where the code hasn't changed - should yield an error. Could we please change the behaviour on this to be a warning rather than an error?

[ghedamat]

hi @mariogintili

thanks for this.

Could you remove the Gemfile.lock commit from this PR?

I see it's doing a major update for jekill and I'd rather have that separate (and checked) as there might be some breaking stuff in there

thanks!

[shellandbull]

hello @ghedamat there isn't a Gemfile.lock in any commit because this isn't a PR, is an issue 😄

[ghedamat]

oh gosh, sorry @mariogintili I meant to answer another PR :p, too many tabs open :D

we had a discussion with @achambers about your issue, I believe he'll post the results here soon :)

[achambers]

@mariogintili After some discussion with the team about this it was decided that, for CI, the default value of allowOverwrite should really be true which would mean you would never find yourself in the situation.

The allowOverwrite config option is really a hangover from 0.4.x where the default revision key strategy was using the git sha, and developers testing deploys from their development environment didn't want to have to commit each time they made a change to test their deploy.

The flag can still be used for this but it is the team's view that a CI environment can be viewed as a controlled environment where you can be reasonably confident that you know the state of your code at a point in time and therefore can set allowOverwrite == true.

It seems that maybe the real change to be made is to the documentation around allowOverwrite, how it came to be, what it is intended for and how it should be used.

Maybe we can use this issue to track that change instead?

[achambers]

Closing based on the last comment in this thread