Detect version mismatches between shard.yml and git tags #341

RX14 · 2020-03-31T18:34:46Z

No description provided.

src/resolvers/git.cr

straight-shoota · 2020-03-31T19:13:56Z

Can you illustrate the motivation for this? Is there a specific reason or just because it feels right? 😄

I had considered such a strict matching and it sure is correct that a tagged release should report the tagged version in the spec file.
But, this is actually not trivial to simply error. This would break installing lots of published shards because shard authors seem to be very lax about that. It probably contributed that shards did never check this, so there have been no errors so far. We don't even provide a tool to automaticaly verify correctness before releasing (see #29).

To put some numbers on this: The database of shardbox.org currently lists 5311 releases, 768 of them report a different version in the spec file than the git tag. That's 14%.

These mismatches are hard to fix retroactively because you would have to revisit each tagged commit, append a fix commit and update the tag. And then the main repo ends up with different tags than its clones.

All in all it would probably be better to not be hypercritical about this. The relevant part for resolving versions is the actual tagged commit. The version property in the spec can be understood as primarily informational to convey this information to the checked out working dir. Ideally, it should be accurate. But it doesn't affect the function of shards when it doesn't match.

RX14 · 2020-03-31T21:53:51Z

These mismatches are hard to fix retroactively because you would have to revisit each tagged commit, append a fix commit and update the tag

This is not true - I made sure to only error if the installed version was wrong. Shard authors would simply need to tag a single new release and have everyone upgrade.

How many of that 14% are the latest release?

The version should either be enforced or removed. It's clearly not actually used in the resolver. I'm fine with warning on this for a release, then making it an error next shards release.

straight-shoota · 2020-03-31T23:01:23Z

Currently 71 shards report a different version in the spec file of their latest release. Out of 822 total shards, that's about 9%.

asterite · 2020-03-31T23:11:11Z

I like this, but I would try to enforce it in some other way. Maybe we can have a shards tag command. But doing it on install can be a bit disruptive.

RX14 · 2020-03-31T23:12:21Z

I think we should warn, at least, then we can discuss switching to an error later.

asterite · 2020-03-31T23:14:37Z

Warning is good. Then authors can be notified of this mismatch.

waj · 2020-04-01T01:46:59Z

What's the function of the version field in shards.yml? I know other dependency managers also have this attribute, but I always wonder what's the real reason behind it. The value is always "wrong" besides on the tag, unless you carefully update it to contain a prerelease value before tagging and don't forget to remove it on the commit finally tagged. In other words, too error prone and not the mandatory field that represents which is the current version.

I'm not strictly saying that we should remove it... I want to understand the motivation to have it.

ysbaddaden · 2020-04-01T07:37:02Z

@waj A version tag freezes a release, but it doesn't mean that it's always "wrong" otherwise. For example:

when mixing refs (tag, commit, branch) and version constraints: we need to know whether the selected refs (branch: some-fix) still matches a version constraint (~> 4.0);
the path resolver needs to find a version;
we can also imagine other resolvers without tags, for example downloading tarballs.
the shards version command also uses it from shard.yml (to void repeating the version everywhere).

TBH I think the version tag came after the version field. It's merely a mean to easily find and extract the release from the Git history.

straight-shoota · 2020-04-01T10:39:53Z

Issuing a warning sounds fine.
Could also consider adding such a warning on shardbox.org as well. Maybe even alert the maintainer.

And for the future it would help to implement #29 and maybe even a tool that completely manages the release process.

waj · 2020-04-01T11:29:48Z

@ysbaddaden yes, I agree, specially with point 1. With "wrong" I mean that if I'm planning to release a version and I update the shard.yml, until I tag the release somehow all the commits have the same version. We found this problematic at Manas for example, when the QA team has to report issues about a deployed version of an app. If the team just look at the version within the code, the reported issues wouldn't be precise about where the bug was found. That's why in our (automated) process we always add some prerelease attribute and the git commit to the version, (1.0.1-dev-abcdef0) unless the deploy was made from an exact tag (1.0.1).

So yes, I agree with this PR but only with a warning. Outdated version in a shard.yml wont affect shard resolution when using tags, and that's when the mismatch will be detected. But it will produce some issues when git refs are used for example.

RX14 · 2020-04-01T11:40:47Z

Well, mismatched versions with tags does break shards check as reported in vladfaust/migrate.cr#15.

I think it'd be better to start disallowing them at a certain point.

waj · 2020-04-01T11:42:18Z

@RX14 but that's a bug we could fix :)

RX14 · 2020-04-01T11:45:05Z

I guess, though I also consider that mismatched versions are accepted a "bug".

RX14 · 2020-04-01T12:09:20Z

Changed the error into a warning.

straight-shoota · 2020-04-01T12:25:56Z

src/molinillo_solver.cr

@@ -71,7 +71,7 @@ module Shards
            raise Error.new("Error shard name (#{spec.name}) doesn't match dependency name (#{dependency.name})")
          end
          if spec.mismatched_version?
-            raise Error.new("Error shard version (#{spec.original_version}) doesn't match tag version (#{spec.version})")
+            Log.warn { "Shard version (#{spec.original_version}) doesn't match tag version (#{spec.version})" }


This message should probably also include dependency.name.

src/molinillo_solver.cr

RX14 · 2020-04-01T12:38:12Z

Two of the same obvious feedback at the same time :)

My bad, the shard name is definitely required.

RX14 · 2020-04-01T15:26:46Z

Should be ready to go now? Shame this didn't make the release though.

bcardiff · 2020-04-01T15:40:55Z

It is expected that there will be another release of shards before 1.0. Let's release what we have today. I'm sending the release PR soon.

RX14 · 2020-04-01T16:13:48Z

or you could press merge :)

bcardiff · 2020-04-01T16:23:17Z

No without reviewing it first (which I hadn't :-) ). There are more changes to come in a next release.

Detect version mismatches between shard.yml and git tags

RX14 added 2 commits March 31, 2020 19:20

Refactor resolvers slightly

5b3975d

Detect version mismatches between shard.yml and git tags

9659ecf

Sija reviewed Mar 31, 2020

View reviewed changes

src/resolvers/git.cr Show resolved Hide resolved

Warn instead of error

27a5aa1

straight-shoota reviewed Apr 1, 2020

View reviewed changes

waj requested changes Apr 1, 2020

View reviewed changes

src/molinillo_solver.cr Outdated Show resolved Hide resolved

Show shard name in warning

b0958c4

straight-shoota approved these changes Apr 1, 2020

View reviewed changes

waj approved these changes Apr 1, 2020

View reviewed changes

waj merged commit 2c98f78 into crystal-lang:master Apr 3, 2020

straight-shoota mentioned this pull request Apr 3, 2020

SPEC: Allow periods in shard name #321

Closed

straight-shoota mentioned this pull request Apr 3, 2020

Tag/version validation #106

Closed

repomaa mentioned this pull request Apr 6, 2020

shards tag command #345

Closed

RX14 mentioned this pull request Apr 10, 2020

Add small check for bad shards #350

Closed

bcardiff added this to the v0.11.0 milestone May 27, 2020

f-fr pushed a commit to f-fr/shards that referenced this pull request Jan 2, 2021

Merge pull request crystal-lang#341 from RX14/feature/vheck-versions

322bd3d

Detect version mismatches between shard.yml and git tags

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detect version mismatches between shard.yml and git tags #341

Detect version mismatches between shard.yml and git tags #341

RX14 commented Mar 31, 2020

straight-shoota commented Mar 31, 2020 •

edited

Loading

RX14 commented Mar 31, 2020 •

edited

Loading

straight-shoota commented Mar 31, 2020 •

edited

Loading

asterite commented Mar 31, 2020

RX14 commented Mar 31, 2020

asterite commented Mar 31, 2020

waj commented Apr 1, 2020

ysbaddaden commented Apr 1, 2020

straight-shoota commented Apr 1, 2020

waj commented Apr 1, 2020

RX14 commented Apr 1, 2020

waj commented Apr 1, 2020

RX14 commented Apr 1, 2020

RX14 commented Apr 1, 2020

straight-shoota Apr 1, 2020

RX14 commented Apr 1, 2020

RX14 commented Apr 1, 2020 •

edited

Loading

bcardiff commented Apr 1, 2020

RX14 commented Apr 1, 2020

bcardiff commented Apr 1, 2020

Detect version mismatches between shard.yml and git tags #341

Detect version mismatches between shard.yml and git tags #341

Conversation

RX14 commented Mar 31, 2020

straight-shoota commented Mar 31, 2020 • edited Loading

RX14 commented Mar 31, 2020 • edited Loading

straight-shoota commented Mar 31, 2020 • edited Loading

asterite commented Mar 31, 2020

RX14 commented Mar 31, 2020

asterite commented Mar 31, 2020

waj commented Apr 1, 2020

ysbaddaden commented Apr 1, 2020

straight-shoota commented Apr 1, 2020

waj commented Apr 1, 2020

RX14 commented Apr 1, 2020

waj commented Apr 1, 2020

RX14 commented Apr 1, 2020

RX14 commented Apr 1, 2020

straight-shoota Apr 1, 2020

Choose a reason for hiding this comment

RX14 commented Apr 1, 2020

RX14 commented Apr 1, 2020 • edited Loading

bcardiff commented Apr 1, 2020

RX14 commented Apr 1, 2020

bcardiff commented Apr 1, 2020

straight-shoota commented Mar 31, 2020 •

edited

Loading

RX14 commented Mar 31, 2020 •

edited

Loading

straight-shoota commented Mar 31, 2020 •

edited

Loading

RX14 commented Apr 1, 2020 •

edited

Loading