Skip to content

Breaking change with update-snapshot property introduction #149

Description

@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.

Activity

  1. jackmatt2 commented on Feb 15, 2023

    @jackmatt2
    Collaborator

    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
  2. uk-taniyama commented on Feb 15, 2023

    @uk-taniyama

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

  3. jackmatt2 commented on Feb 16, 2023

    @jackmatt2
    Collaborator

    @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.

  4. jackmatt2 commented on Feb 16, 2023

    @jackmatt2
    Collaborator

    I have put in a fix for this in 4.0.5

    I'll now move this to a discussion

  5. locked and limited conversation to collaborators on Feb 16, 2023
  6. converted this issue into a discussion #151 on Feb 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions