Breaking change with update-snapshot property introduction

[lanwen]

Hello,

I updated recently from 4.0.2 to 4.0.3 and immediately got a failure on CI due to snapshot tests, which I suppose shouldn't be the case for the patch release when nothing changed except the dependency.

Tests started to complain on

au.com.origin.snapshots.exceptions.SnapshotExtensionException: isCI=true & update-snapshot=Optional[false]. Updating snapshots on CI is not allowed

as we specify updateSnapshot=false in our gradle config by default

I think the reason for that is the logic introduced with this change here: 194f256#diff-6c0dc58e8f7f6174ee8150ac6b242ba7bbebecf5ed998c9f786cf056163aa189R122
(#145)

It checks only for the presence of the property, but not the value, whereas before absence and the false value were equal.

Perhaps, you could bring the false value for the old property to be considered as none? That would be much appreciated to allow the smooth migration process.

p.s. the new flow with the property value change seems not obvious to me - would you mind describing the way you see it? As I'm always afraid to accidentally commit the property file after changing it to all. Thank you in advance.

[jackmatt2]

Yes - you're correct in that this change should not have broken anything in 4.X release. I'll need to look into it.

Regarding the flow - I see it like this

• By default, the CI env var will not allow updating snapshots. It ignores what is defined in snapshot.properties
• It should not be necessary to explicitly define it as false for CI environments

If you accidentally checkin the change tosnapshot.properties you are protected in two ways

• CI will ignore what you specified in snapshot.properties and still fail the tests
• PR reviews and identify the mistaken change to the file

[uk-taniyama]

It would be better to have external control over the updateSnapshot.
It is easier to control if the environment variable -> snapshot.properties.

[jackmatt2]

@uk-taniyama it's currently not an environment variable - it's a system property. An environment variable would make it easier for people to control. But I'm not too enthusiastic to now support 3 different ways of accomplishing this.

[jackmatt2]

I have put in a fix for this in 4.0.5

I'll now move this to a discussion