-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Revert "Merge pull request #3336 from rtfd/use-active-for-stable" #3368
Conversation
@@ -707,7 +707,7 @@ def update_stable_version(self): | |||
if current_stable: | |||
identifier_updated = ( | |||
new_stable.identifier != current_stable.identifier) | |||
if identifier_updated and current_stable.active: | |||
if identifier_updated and current_stable.active and current_stable.machine: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, comparing this with the original code, we are just adding a another condition (has to be active
) but we keep the machine
.
In other words, we are just updating the stable version only if, the identifier changed (a new tag become the stable, for example) and the stable version is active and it was created by RTD.
If what I'm saying is correct, I think we are OK :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, thats the goal :)
@@ -391,7 +388,7 @@ def test_update_inactive_stable_version(self): | |||
|
|||
version_stable = Version.objects.get(slug=STABLE) | |||
self.assertFalse(version_stable.active) | |||
self.assertEqual(version_stable.identifier, '0.9') | |||
self.assertEqual(version_stable.identifier, '1.0.0') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this test be for 0.9
? Or is this test being repurposed to check the new logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, that was the original fix.
|
||
Version.objects.create( | ||
project=self.pip, | ||
identifier='foo', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
origin/stable
here I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm tested it gets updated from a prior "set" version.
Ready for re-review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
This reverts commit 0d9c3fe, reversing
changes made to 73b693c.