Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support shard renames #327

Merged
merged 3 commits into from
Mar 26, 2020
Merged

Support shard renames #327

merged 3 commits into from
Mar 26, 2020

Conversation

waj
Copy link
Member

@waj waj commented Mar 10, 2020

Currently a shard that was renamed in some point in history cannot be installed unless a version is specified. This is annoying, because for example this shard.yml fails with a misleading error:

name: test
version: 0.1.0

dependencies:
  lucky:
    github: luckyframework/lucky
$ shards install
Resolving dependencies
Fetching https://github.com/luckyframework/lucky.git
Error shard name (lucky_web) doesn't match dependency name (lucky)

This PR allows installing latest version of a shard (when no version is specified) or a previous version only if the name matches.

@straight-shoota
Copy link
Member

straight-shoota commented Mar 10, 2020

This basically implements my proposal from #310 (comment) so I agree with that change as it seems like the most expected behaviour and avoids serious ux issues.

We should still consider the critical voices from #310 and especially make sure that edge cases are handled properly.
For example: when foo was renamed to bar and the the dependency specifies foo with no version restriction, it seems like the current implementation would pick the latest release with the name foo. I don't think that's correct. It should still pick the latest version overall and then error when that version doesn't match the requested name. If you want to use an older version with foo name, you should have to explicitly specify a version.

Please also see my suggested implementation in straight-shoota:feature/renamed-shard.

@waj
Copy link
Member Author

waj commented Mar 11, 2020

I wasn’t aware of that conversation. Thank you for pointing it out. I think it makes sense but I’m not sure the implementation is that simple like yours. Anyway, tomorrow I’ll analyze it and update this PR.

@bcardiff
Copy link
Member

We need to document the following known issue: A recently renamed shard must have an explicit branch or ref requirement. Otherwise since the lookup of all available versions and filtering for matching new name leads to an empty list, there is no installation possible.

@waj
Copy link
Member Author

waj commented Mar 11, 2020

@straight-shoota I just updated the PR and the description

@straight-shoota
Copy link
Member

A recently renamed shard must have an explicit branch or ref requirement. Otherwise since the lookup of all available versions and filtering for matching new name leads to an empty list, there is no installation possible.

I don't follow. Why can't the available versions be limited to those matching the dependency name? This should at least work for the name at HEAD.

@waj Could you please add specs for using both old_name and new_name as a dependency without a version restriction? And the same for another dependency where the only version with new_name is not tagged.

@bcardiff
Copy link
Member

@straight-shoota after the push after my comment the implementation and the test_install_new_when_shard_was_renamed spec deals with that case. Before the filtering of versions was done in another place leading to the not supported scenario.

@straight-shoota
Copy link
Member

Oops, I missed that.
Still it would be great to have specs for untagged head and failure specs for new_name@0.1.0, old_name and old_name@0.2.0

@waj
Copy link
Member Author

waj commented Mar 12, 2020

@straight-shoota I just added the specs you requested. For the untagged head, it works when the "branch" is specified, because there is already tags in the repository. Actually I think this should be always the case, and the switch to "git ref" mode shouldn't be automatic when there is no tags, but that's a topic for another PR 🙂

@straight-shoota
Copy link
Member

Looks good to me.

But I'd like to get feedback from @ysbaddaden and @RX14 as well because of the concerns voiced in #310.

@RX14
Copy link
Contributor

RX14 commented Mar 21, 2020

My concern was that it wasn't worth implementing cause of the complexity. This is a small diff, I think it's up to @ysbaddaden now.

@waj waj merged commit 124c31a into crystal-lang:master Mar 26, 2020
@waj waj deleted the feature/renamed-shards branch March 26, 2020 22:50
@bcardiff bcardiff added this to the v0.10.0 milestone Mar 27, 2020
f-fr pushed a commit to f-fr/shards that referenced this pull request Jan 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants