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.

[uk-taniyama]

Thanks for your reply.
I will use MySnapshotConfig.class like this.

import java.util.Optional;

import au.com.origin.snapshots.SnapshotProperties;
import au.com.origin.snapshots.config.PropertyResolvingSnapshotConfig;
import au.com.origin.snapshots.exceptions.MissingSnapshotPropertiesKeyException;

public class MySnapshotConfig extends PropertyResolvingSnapshotConfig {
    private static final Optional<String> all = Optional.of("");
    private static final Optional<String> none = Optional.empty();

    @Override
    public Optional<String> updateSnapshot() {
        if (isCI()) {
            return none;
        }

        try {
            String updateSnapshotEnvVar = SnapshotProperties.getOrThrow("update-snapshot-env-var");
            if (updateSnapshotEnvVar != null && !updateSnapshotEnvVar.isEmpty()) {
                String updateString = System.getenv(updateSnapshotEnvVar);
                if (updateString == null || updateString.equals("none")) {
                    return none;
                }
                if (updateString.equals("all")) {
                    return all;
                }
                return Optional.of(updateString);
            }
        } catch (MissingSnapshotPropertiesKeyException ignore) {
        }
        return super.updateSnapshot();
    }
}

[jackmatt2]

I have put in a fix for this in 4.0.5

I'll now move this to a discussion

[muehmar]

Actually, I was pretty happy with the old way to update snapshots by passing -PupdateSnapshot to the test task. Sure, it was necessary to use some gradle tricks to get it working, but once it worked, it was pretty convenient. Personally, I dont really like the way it is implemented now with setting the property in the file to all and setting it back again to none. Is there a way you could think of to support as alternative to modify files to update snapshots? Or perhaps still support the old way (I know its still supported but I suspect you will remove it with the next major version)?