concourse / semver-resource

automated semantic version bumping
Apache License 2.0
97 stars 105 forks source link

Remove GTE conditional from check routine to support rollback scenarios #113

Closed glompix closed 3 years ago

glompix commented 4 years ago

I have a scenario where a resource is tracked via semver. If something goes awry, I may want to process against an earlier version of this resource via pinning it and re-running the task. Currently there's no way to do this due to the v.GTE guard.

taylorsilva commented 4 years ago

Thanks for the PR @glompix and sorry for the delay in getting to this. I'll be reviewing this today :)

taylorsilva commented 4 years ago

@glompix Can you explain your use-case in more detail? I'm not able to understand why this change is desirable.

I wanted to see how the resource currently behaves so I made this pipeline:

resources:
- name: version
  type: semver
  icon: tag
  source:
    driver: gcs
    bucket: concourse-artifacts-test
    json_key: ((concourse_artifacts_json_key))
    key: test-get-version

jobs:
- name: version-bump
  plan:
  - get: version
  - put: version
    params:
      bump: major

I ran the job once to go from 0.0.0 to 1.0.0.

image

I then pinned the job to 0.0.0 and ran it again. The get step returned the pinned version as expected. The only undesirable part I can see was that the put step bumped the version again.

image image

The job could be modified to avoid this scenario. It would depend on your use-case if that even makes sense though.

image
taylorsilva commented 4 years ago

@glompix Hey, I see your status is set to "busy". Could you let us know if you plan to come back to this PR at some point? No need to answer the outstanding question and PR checks :)

glompix commented 4 years ago

Sorry it took so long to come back to this. It feels like forever ago!

Another of my use cases was that I may want to frequently release a prerelease version like 1.0.0-rc1-012def, before i'm ready for the final 1.0.0. i may even want to go back and forth, deploying between versions like 2.0.0-experimental-012def and 1.x.y without having to keep pinning every time and maybe manually invoking the job. We don't actually useput. We just feed in images/versions from another system, or even from a developer's machine.

I guess my question would be, why have the GTE in the first place?

taylorsilva commented 4 years ago

Your use-case makes sense to me! Having the GTE conditional does make this resource very restrictive to use from what I've been able to think through. From what I can tell it seems to be there mostly as a guardrail. Maybe we should just remove the guardrail? Maybe it gets in people's way more often than it really helps protect them from... idk really haha

I want to get some second opinions before removing this.

@vito any thoughts on keeping/removing this conditional?

vito commented 4 years ago

@taylorsilva The idea behind the GTE check is that resource check operations are typically to check for something 'equal to or newer' than the given version. So it seemed to make sense to add there, but to be honest it's definitely a bit weird in situations where the version has rolled back, so I think it may be OK to remove it.

I think there's some kind of strange behavior to this resource type overall, tbh. It's not representing a history of versions, as resources are supposed to; there's really just one version at a time. 🤷‍♂️ I think it'll take time and effort to come up with something more natural though. So I'm OK with just removing the check.

glompix commented 4 years ago

thanks for the feedback! i can take care of this on the weekend probs