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

Allow git tags with `/` in the name and properly slugify #3545

Merged
merged 2 commits into from Jan 30, 2018

Conversation

Projects
None yet
3 participants
@humitos
Member

humitos commented Jan 23, 2018

Closes #1989

@humitos humitos requested a review from agjohnson Jan 23, 2018

@stsewd

This comment has been minimized.

Member

stsewd commented Jan 23, 2018

Related #2563 (comment)

@stsewd

This comment has been minimized.

Member

stsewd commented Jan 24, 2018

My above comment is about an issue with branches, no tags. So, can we allow / on branches names/slugs too?

@humitos

This comment has been minimized.

Member

humitos commented Jan 24, 2018

@stsewd I think what you mentioned is probably another issue.

I mean, there two things here:

  • tag/branch name that is returned by the parse_tags/branches method
  • verbose_name that is saved in the database

It seems that some time ago, we weren't using VersionSlugFied so the slug was created in a hacky way all around the code.

Anyway, thanks to your comment I found that the parse_branches was doing what the VersionSlugField should be doing but in a hacky way. So, I updated that code to return the proper name of the branch instead of a manipulated one.

@humitos

This comment has been minimized.

Member

humitos commented Jan 24, 2018

I suppose that my last commit should fix the comment that you linked because the slug is used to build the URL of the documentation but the verbose_name is used to create the link to Edit on Github. @stsewd can you confirm this?

@stsewd

This comment has been minimized.

Member

stsewd commented Jan 25, 2018

@humitos yes, this solves the problem.

@ericholscher

This comment has been minimized.

Member

ericholscher commented Jan 25, 2018

Will this cause existing versions to be deleted and re-created? What happens with existing active versions that would be "re-tagged"? This should likely include a data migration, as with all changes to our slugging, but that will also breaking existing URL's to active versions, so we'll likely need redirects too.

@humitos

This comment has been minimized.

Member

humitos commented Jan 26, 2018

Will this cause existing versions to be deleted and re-created?

I don't think so. We are using identifier to decide if the tag/branch comming from git is the same or not and this shouldn't touch the identifier.

if version_name in list(old_versions.keys()):
if version_id == old_versions[version_name]:
# Version is correct
continue

What happens with existing active versions that would be "re-tagged"?

I suppose that this won't happen.

This should likely include a data migration, as with all changes to our slugging, but that will also breaking existing URL's to active versions, so we'll likely need redirects too.

A couple of things here:

  • we can't make a data migration because we don't have the full tag name since we were dropping it. I mean release/foobar was converted to foobar when parsing the git output, so we don't have a way to re-construct the missing part
  • existing version slugs won't be updated because of the identifier mentioned above; verbose_name won't be updated and slug either
  • Broken "Edit on GitHub" links will still be broken
  • new versions will have the new verbose_name, which will create a better slug and the "Edit on Github" link will work

So, I'd say that it's safe to merge this PR but I would like to know if my thoughts are right and write a couple of tests with the scenario described in the bullets. What do you think? Do you see any other case that I'm not considering?

@ericholscher

This comment has been minimized.

Member

ericholscher commented Jan 30, 2018

Yea. I see how this is a better approach. I still worry it will cause active versions that came from the pre-existing broken names to not be updated properly.

Was there a bug where we weren't properly updating these versions because the post-commit logic was parsing them properly, but the VCS code wasn't?

@ericholscher

This comment has been minimized.

Member

ericholscher commented Jan 30, 2018

Also, Versions should really have slugs, heh.

@ericholscher ericholscher merged commit cba3a99 into master Jan 30, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@agjohnson agjohnson deleted the humitos/backend/git-tags branch Jan 30, 2018

@humitos

This comment has been minimized.

Member

humitos commented Jan 30, 2018

I'm not 100% sure to understand your comments, but

I still worry it will cause active versions that came from the pre-existing broken names to not be updated properly.

Mmm... Yes. This won't be updated automatically. The sync versions is not too aggressive and do nothing if the identifier doesn't change but, what we can do here? I'm not sure how to fix already broken names.

Also, Versions should really have slugs, heh.

😕, what do you mean here? Versions have slugs, it's populated from verbose_name though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment