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

Delete tags with same commit #4915

Merged
merged 4 commits into from
Nov 20, 2018

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Nov 19, 2018

This only happens with tags, because they are basically an alias for a commit, so it can be several tags pointing to the same commit or identifier.

We exclude branches and tags by its identifier

https://github.com/rtfd/readthedocs.org/blob/4031b9078efba9948301e89c948a236671086d26/readthedocs/restapi/utils.py#L144-L144

I can confirm that the other VCS share the same concept for tags

Git: we already know we can have several tags pointing to the same commit

Mercurial: I didn't find anything about this in the docs https://www.mercurial-scm.org/wiki/Tag, but here says that it's possible http://hgbook.red-bean.com/read/managing-releases-and-branchy-development.html

There's no limit on the number of tags you can have in a repository, or on the number of tags that a single revision can have

Bazaar: From the docs http://wiki.bazaar.canonical.com/Specs/Tagging

The option can be repeated to add multiple tags pointing to the same revision.

Subversion: it doesn't support tags

Fix #4875

@stsewd stsewd added the PR: work in progress Pull request is not ready for full review label Nov 19, 2018
@codecov
Copy link

codecov bot commented Nov 19, 2018

Codecov Report

Merging #4915 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4915      +/-   ##
==========================================
- Coverage   76.65%   76.64%   -0.01%     
==========================================
  Files         158      158              
  Lines       10059    10055       -4     
  Branches     1269     1267       -2     
==========================================
- Hits         7711     7707       -4     
  Misses       2007     2007              
  Partials      341      341
Impacted Files Coverage Δ
readthedocs/restapi/utils.py 90.99% <100%> (-0.32%) ⬇️

@@ -105,12 +105,13 @@ def test_new_tag_update_active(self):
self.pip.get_stable_version().identifier,
)

def test_new_tag_update_inactive(self):
def test_new_tag_dont_update_inactive(self):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally, I was able to fix and understand why this test was failing, first this test is wrong, why? Because our current logic says (and do)

https://github.com/rtfd/readthedocs.org/blob/4031b9078efba9948301e89c948a236671086d26/readthedocs/restapi/views/model_views.py#L176-L177

why the test was passing?

Because the version was created with type='unknown', so it was being skipped here

https://github.com/rtfd/readthedocs.org/blob/4031b9078efba9948301e89c948a236671086d26/readthedocs/projects/version_handling.py#L238-L240

Why the test fail now?

Because we are filtering by branches and tags only, so the unknown version was being deleted.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, good catch!

@stsewd stsewd removed the PR: work in progress Pull request is not ready for full review label Nov 20, 2018
@stsewd stsewd requested a review from a team November 20, 2018 00:29
Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This explains the behavior I was seeing in prod when trying to test the tag setup. 💯

@@ -105,12 +105,13 @@ def test_new_tag_update_active(self):
self.pip.get_stable_version().identifier,
)

def test_new_tag_update_inactive(self):
def test_new_tag_dont_update_inactive(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, good catch!

@stsewd
Copy link
Member Author

stsewd commented Nov 20, 2018

Tested this locally, merging. I'll put this to the deploy checks, I guess that our docs have some old tags from the pass release, right? If so, when building our docs those tags should disappear p:

@stsewd stsewd merged commit c37b009 into readthedocs:master Nov 20, 2018
@stsewd stsewd deleted the delete-tags-with-same-commit branch November 20, 2018 16:24
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.

3 participants