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

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

Merged
merged 2 commits into from Jan 30, 2018

Conversation

@humitos
Copy link
Member

@humitos humitos commented Jan 23, 2018

Closes #1989

@stsewd
Copy link
Member

@stsewd stsewd commented Jan 23, 2018

Related #2563 (comment)

Loading

@stsewd
Copy link
Member

@stsewd 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?

Loading

@humitos
Copy link
Member Author

@humitos 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.

Loading

@humitos
Copy link
Member Author

@humitos 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?

Loading

@stsewd
Copy link
Member

@stsewd stsewd commented Jan 25, 2018

@humitos yes, this solves the problem.

Loading

@ericholscher
Copy link
Member

@ericholscher 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.

Loading

@humitos
Copy link
Member Author

@humitos 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.

https://github.com/rtfd/readthedocs.org/blob/1af444173481df9a7b16e015fc6b12abe8155e7e/readthedocs/restapi/utils.py#L27-L30

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?

Loading

@ericholscher
Copy link
Member

@ericholscher 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?

Loading

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Jan 30, 2018

Also, Versions should really have slugs, heh.

Loading

@ericholscher ericholscher merged commit cba3a99 into master Jan 30, 2018
1 check passed
Loading
@agjohnson agjohnson deleted the humitos/backend/git-tags branch Jan 30, 2018
@humitos
Copy link
Member Author

@humitos 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.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants