drone-docker
drone-docker copied to clipboard
drone-plugins
DefaultTags() returns non-semver tags as-is.
The previous behavior of DefaultTags() causes problems to us because our staging environment depends on (ie. pulls) the latest images so whenever not a semver tag hits the repository, auto_tag eventually moves the latest tag to another arbitrary image in the repository.
IMHO it would be better to make the build fail or use the provided tag as is.
According to @bradrydzewski note https://github.com/drone-plugins/drone-docker/issues/237#issuecomment-627393512 DefaultTags provides tags as-is in such cases.
IMHO it should fail if it's not a semver tag. For cases without a semver tag somebody should not use auto tagging.
@tboerger you must better know how auto-tagging and semantic versioning are supposed to work in drone-docker. However, I would rather like to see the semver support as an optional feature on top of auto-tagging because in this case auto-tagging could be used in more scenarios (like mine). As it stands now, I nearly duplicate all the functionalities of auto-tagging in my drone pipeline configuration just because of that issue.
This was the reason why, as I stated in the description, I followed @bradrydzewski comment in this pull request.
I shared my opinion. IMHO the advantage of the autotagging is the automated split of the versions. For taking the tag as it is nobody needs autotagging, just use the drone env variables like they are.
IMHO it would be better to make the build fail or use the provided tag as is. IMHO it should fail if it's not a semver tag
I agree with both of you that ideally it would fail (or just skip the step similar to how we handle pull requests) if semver parsing failed. The reason I suggested using the tag as-is instead of failing is because I was concerned failing or skipping could inadvertently break existing pipelines that do not expect this behavior -- although perhaps this is not a practical concern and I am being overly cautious.
Either way I agree we need to change the logic, either as implemented in this pull request, or by failing or skipping. I view the current behavior as a bug. I view this pull request as an improvement, even though we may be able to improve it even further.
Just taking the tag like it is would also be some kind of breaking change... It would silently build another tag as the people out there might already expect ;)
I would be more than happy if you merge it.
However, if you think I could add a tags.semver boolean option to the cli.Context to be more backward-compatible and provide some control. It would be true by default meaning it skips the entire build (return nil before calling plugin.Exec() - I suppose this is what @bradrydzewski meant by skipping). Otherwise, it would enable us to use the tag as is.
I appreciate the option, but would prefer we avoid adding more configuration parameters to the plugin. Since any change we make is going to be breaking, perhaps we just bite the bullet and modify the behavior to fail the plugin if semver parsing fails. Draft of this change at https://github.com/drone-plugins/drone-docker/pull/279.