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

stable version stuck on a specific commit #3913

Merged
merged 41 commits into from Jun 19, 2018

Conversation

@stsewd
Copy link
Member

@stsewd stsewd commented Apr 5, 2018

There are several reports of users that somehow ended with the stable version stuck on a specific commit, and the only solution is to recreate the project on RTD.

I was able to reproduce this issue and other (no relevant, since it can be fixed without recreating the project). Those bugs are documented here #3887.

Here I'm exposing this bug #3887 (comment).

When a user creates a tag named stable, the RTD's stable loses its machine attribute, and it can't be recreated (even deleting the tag).

This will close #3887, close #534, close #2032, close #1785, close #3041

@stsewd
Copy link
Member Author

@stsewd stsewd commented Apr 5, 2018

I going to try to find the root of the problem and fix it here.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Apr 5, 2018

I saw that there is a test case that allows the user to re-define the stable version
https://github.com/rtfd/readthedocs.org/blob/950f732f7a82f31ff9545bdb58740a4033e5b9b4/readthedocs/rtd_tests/tests/test_sync_versions.py#L544

But the problem with that is that if the user wants to delete that version later, it can't.

@stsewd stsewd force-pushed the stuck-stable branch 2 times, most recently from 32e8c0e to 2ab9257 Apr 6, 2018
@stsewd stsewd force-pushed the stuck-stable branch 2 times, most recently from e6e6e5e to de0d86f Apr 8, 2018
@RichardLitt RichardLitt changed the title Stuck stable stable version stuch on a specific commit Apr 9, 2018
@RichardLitt RichardLitt changed the title stable version stuch on a specific commit stable version stuck on a specific commit Apr 9, 2018
)
self.assertEqual(resp.status_code, 200)

self.pip.refresh_from_db()
Copy link
Member Author

@stsewd stsewd Apr 10, 2018

I'm not sure if this is necessary here.

Copy link
Member

@humitos humitos Apr 10, 2018

As a general rule:

refresh_from_db is needed when you have a db object already in memory and the registry in the db is changed by an external reason (celery, api request, HTTP post request, etc). So, in that case, the object in memory has to be refreshed from the db to have the new values.

Copy link
Member

@humitos humitos Apr 10, 2018

Related fields (those one that generate new queries to db) are not affected by this. For example, self.pip.versions.all() will hit the db and it's not affected by an old self.pip in memory.

self.assertFalse(version_9.active)

# Did update to user-defined stable version
version_stable = Version.objects.get(slug='stable')
version_stable = self.pip.versions.get(slug='stable')
Copy link
Member Author

@stsewd stsewd Apr 10, 2018

The version can collide with another fixture project

@stsewd
Copy link
Member Author

@stsewd stsewd commented Apr 10, 2018

Same bug when a user creates a latest branch/tag. Also, there aren't tests about redefining the latest version.

# is created.
self.pip.save()

def test_user_defined_latest_version_tag(self):
Copy link
Member Author

@stsewd stsewd Apr 10, 2018

This test is failing. When the user creates a tag named latest, a new version is created latest_a. Is that the desired behavior?

/cc @humitos

Copy link
Member

@humitos humitos Apr 10, 2018

I'm thinking about this.

  • If the user creates a latest and we just change our machine to False: that project looses the ability to build the latest changes in their repo. I mean, new commits to Project.default_branch won't trigger any build. Is that what we want?

    • this behaviour seems to be compatible with what the stable branch will do in a similar case
  • having latest and latest_a seems to be confusing to me. Also, supposing that you want to point your readers to your latest: the url will be /latest_a which probably people won't like anyway

  • I'm not sure if we have some chunk of code dependent/assuming that we will always have a LATEST Version with machine=True --that's a good checking to do before make a decision.

    • can you check this?

For now, it seems that what we want is the first idea of just changing the machine field to False. I'd like more input here, though.

Copy link
Member Author

@stsewd stsewd Apr 10, 2018

I didn't find any query that depends on machine=True for latest, but I did find some interesting relation between latest and default_branch on the code

https://github.com/rtfd/readthedocs.org/blob/b7745cd21a787c5ef14d127e34613bb3aaa2f52b/readthedocs/core/views/hooks.py#L36-L44

https://github.com/rtfd/readthedocs.org/blob/b7745cd21a787c5ef14d127e34613bb3aaa2f52b/readthedocs/projects/models.py#L792-L802

https://github.com/rtfd/readthedocs.org/blob/b7745cd21a787c5ef14d127e34613bb3aaa2f52b/readthedocs/builds/models.py#L120-L125

https://github.com/rtfd/readthedocs.org/blob/b7745cd21a787c5ef14d127e34613bb3aaa2f52b/readthedocs/projects/models.py#L315-L319

https://github.com/rtfd/readthedocs.org/blob/b7745cd21a787c5ef14d127e34613bb3aaa2f52b/readthedocs/projects/models.py#L359-L361

I think the use case of having a custom latest is covered by the default branch setting

screenshot-2018-4-10 edit advanced project settings read the docs

BUT, what happens when the user wants a commit/tag as the latest version (very weird though)?

I did a quick test with a commit and a tag, nothing breaks (even if they are marked as a branch).

So, the final question, how to handle this when a user creates a tag named latest? If the users want the URL to be /latest/, then putting the tag name on the default_branch setting can achieve this.

Copy link
Member

@humitos humitos Apr 16, 2018

OK, so it seems that what we want is:

  • if the user defines a latest branch, remove the machine=True and handle it as a common branch
  • if the user wants to trigger new builds on a non-default-latest branch, he can change the default_branch in the admin
  • do not create a latest_a if the users defines a latest branch/tag on the repo

Does these thoughts answer your questions and unblock you to continue with this PR?

Copy link
Member Author

@stsewd stsewd Apr 16, 2018

What would happen if a user creates a latest tag and also touch the setting for default_branch? (very extreme case, I don't know if even worth considering p:)

According to

https://github.com/rtfd/readthedocs.org/blob/b7745cd21a787c5ef14d127e34613bb3aaa2f52b/readthedocs/projects/models.py#L315-L319

Her/his initial latest would change.

Anyway, I'm going to write more tests to check the requirements, but I think these changes are going to break others parts of the code that depends on latest, hope not.

@@ -152,6 +152,484 @@ def test_new_tag_update_inactive(self):
version_8 = Version.objects.get(slug='0.8.3')
self.assertFalse(version_8.active)

def test_normal_behavior_for_stable_after_deleting_user_defined_tag(self):
Copy link
Member

@humitos humitos Apr 16, 2018

Can you use pytest.mark.xfail(strict=True) here? That way, these test will fail and pytest won't fail but pytest will fail when you write the logic and they start passing.

on the user repository, the RTD's ``stable`` is back.
"""
# There isn't a stable version yet
self.pip.versions.exclude(slug='master').delete()
Copy link
Member Author

@stsewd stsewd Apr 17, 2018

OK, this doesn't feel quite right. But all tests are using the pip project... Should I start using a dynamic fixture instead or should I use something else? /cc @humitos

Also, there aren't tests for new projects with already created tags/branches with latest and stable. I'm going to add them (maybe is kind of more convenient to use the dynamic fixtures because more tests are going to use it).

Copy link
Member

@humitos humitos Apr 17, 2018

You can remove that version inside your test and it won't affect the others since the fixture is loaded one each test: https://docs.djangoproject.com/en/2.0/topics/testing/tools/#topics-testing-fixtures

Copy link
Member Author

@stsewd stsewd Apr 17, 2018

Yeah, but I was saying about is that feels weird to do that setup inside the test

Copy link
Member

@humitos humitos left a comment

Good work! I think this it's a really great fix and very good addition of test cases :)

Some things to consider:

  • use hardcoded values for assertEqual
  • check machine status after the first sync in most of the test cases
  • use reverse to create the URLs
  • remove the assertNotEqual since they are not needed
  • improve the names of the test cases

Finally, another major refactor that we can consider here since most of the test cases share similar code is to use pytest parametrize: https://docs.pytest.org/en/latest/parametrize.html

(I'm not sure if it's possible, though)

stable_version = (
project.versions
.filter(slug=STABLE, type=type)
.first()
Copy link
Member

@humitos humitos Apr 17, 2018

Instead of .first you can use .exists here, since it's exactly what we need.

Copy link
Member Author

@stsewd stsewd Apr 17, 2018

But we need the object on the next line

for version in versions:
version_id = version['identifier']
version_name = version['verbose_name']
if version_name == STABLE_VERBOSE_NAME:
has_user_stable = True
Copy link
Member

@humitos humitos Apr 17, 2018

Since we are using has_user_stable outside this loop, I think it's preferable to create it right before where it's used: line 63 using.

Copy link
Member Author

@stsewd stsewd Apr 17, 2018

But is still outside the loop, so it's really not right before it's used.

Copy link
Member

@humitos humitos Apr 17, 2018

Yes. Is is used inside the loop?

Copy link
Member Author

@stsewd stsewd Apr 18, 2018

Nop, but the logic that came behind is not related to that code.

def test_normal_behavior_for_stable_after_deleting_user_defined_tag(self):
"""
The user creates a tag named ``stable`` on an existing repo,
when syncing the versions, the RTD's ``stable`` is lost
Copy link
Member

@humitos humitos Apr 17, 2018

is lost (set to machine=False) here, to be more explicit.

The user creates a tag named ``stable`` on an existing repo,
when syncing the versions, the RTD's ``stable`` is lost
and doesn't update automatically anymore, when the tag is deleted
on the user repository, the RTD's ``stable`` is back.
Copy link
Member

@humitos humitos Apr 17, 2018

same here: is back (set to machine=True) here, to be more explicit

self.assertEqual(
current_stable.identifier,
version_stable.identifier
)
Copy link
Member

@humitos humitos Apr 17, 2018

Immediately after this, we want to know is that the current_stable.machine is False since now it's a version/tag managed by the user.

Copy link
Member

@humitos humitos Apr 17, 2018

and the current_stable.identifier should be 1abc2def3

Copy link
Member Author

@stsewd stsewd Apr 17, 2018

I believe that is covered by other tests, I'm on the cellphone, so not sure if that is the case

# 0.8.3 is the current stable
self.assertEqual(
version8.identifier,
current_stable.identifier
Copy link
Member

@humitos humitos Apr 17, 2018

Same here.

self.assertEqual(
current_stable.identifier,
version_stable.identifier
)
Copy link
Member

@humitos humitos Apr 17, 2018

Check for machine status here and use hardcoded identifier.

current_stable = self.pip.get_stable_version()
self.assertEqual(
version8.identifier,
current_stable.identifier,
Copy link
Member

@humitos humitos Apr 17, 2018

Same here.

Also, if we hardcode here the assertNotEqual is not necessary.

self.assertTrue(version_latest.machine)

@pytest.mark.xfail(strict=True)
def test_normal_behavior_for_latest_after_deleting_user_defined_branch(self):
Copy link
Member

@humitos humitos Apr 17, 2018

Are we blocked on a decission to make before making this test to pass?

Copy link
Member Author

@stsewd stsewd Apr 17, 2018

Nope, just trying to understand all cases for stable first

self.pip.get_stable_version().identifier
)
# There arent others stable slugs like stable_a
other_latest = self.pip.versions.filter(
Copy link
Member

@humitos humitos Apr 17, 2018

other_stable?

Copy link
Member Author

@stsewd stsewd Apr 17, 2018

Ups, copy paste

@stsewd
Copy link
Member Author

@stsewd stsewd commented Apr 18, 2018

OK, this is weird... Tests fail after renaming the tests (nothing else change!). Same thing happening on my local instance, if I checkout to a previous commit everything works. Also if I only run the tests for the single file, everything works (on the same commit were the tests fail).

@stsewd
Copy link
Member Author

@stsewd stsewd commented Apr 18, 2018

I think this is something caused by urls collision, the apiv1 and v2 share the same url name and don't have a namespace.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Apr 18, 2018

I did a quick test putting a namespace to the restapi app, again running the tests only on that file works, running all the project, tests fail.

NoReverseMatch: u'restapi' is not a registered namespace
- tox

@stsewd
Copy link
Member Author

@stsewd stsewd commented Jun 12, 2018

Ok, I have some news... Because the current fetch command doesn't delete untracked tags (only untracked branches), we need to wipe the environment when we delete the duplicated tag (if we delete the duplicated branch, there is no problem).

There are some solutions to delete untracked tags (require more than 2 commands). But I find that the newest version of git has the --prune-tags option, which is used as git fetch --prune --prune-tags (git >2.17).

What should I do? I have some ideas to solve this:

  1. Update git on the servers (we use 2.7.4) and change the fetch command
  2. Revert the exception raised and silently save just one version (branch type)
  3. Expand the error message to explain to the user that should wipe the environment
  4. Wipe the environment automatically if we detect a duplicate version (I don't know if this is simple, I'll look at the code).

I think we want 3 or 4 at the moment and raise an issue to keep track of 1.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Jun 12, 2018

I'm trying to fix the problem by cleaning the current repo in the last commit, tests pass and the user doesn't have to wipe the environment manually, not sure if it's the best way.

(I tested this manually too btw)

@stsewd
Copy link
Member Author

@stsewd stsewd commented Jun 12, 2018

Now, I'd like to communicate this to the user in a friendly way so they can manage/solve this problem by themselves but also for us to remember the whole conversation here and know what are the manual steps to solve it. Otherwise, in two months we will forget this level of details.

We should discuss that in other issue? I don't think there is a quickly solution for that, I mean we can solve the original problem or document this or raise a proper exception when the checkout step fails (but probably the message needs to be generic, I'm not sure, what else can throw an exception here?)

@humitos
Copy link
Member

@humitos humitos commented Jun 18, 2018

I think we want 3 or 4 at the moment and raise an issue to keep track of 1.

Agree with you. Let's do 3 and open a new issue for 1.

We are moving servers (and upgrading Ubuntu version also) and I'm not really sure how much overhead will introduce to upgrade to a different git version but I don't want to add another thing right now on this server migration.

(I tested this manually too btw)

While testing this manually consider that in production we have 4 build servers. So, one scenario that you want to consider is that your code should not depend on the repo that lives on disk.

It could happen that you import a project (that is done by build01). Then, you build latest and that build could be executed by build01 which doesn't have a copy of the repo and has to clone it for the first time.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Jun 18, 2018

I'm trying to change the message here https://github.com/rtfd/readthedocs.org/pull/3913/files#diff-4787d302d24c7ac7a89ad7587dd94007R47 to point the user to wipe the environment, but I'm not sure the best way of doing it, a new user probably doesn't know what wipe the environment is... The message is a constant, so we can't put a link to the wipe page. Maybe a link to the docs https://docs.readthedocs.io/en/latest/guides/wipe-environment.html? @humitos help :(

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Jun 18, 2018

I feel like we're going down a bit of a rabbit hole here. Let's get this shipped and worry about these small details later :)

@humitos
Copy link
Member

@humitos humitos commented Jun 18, 2018

to point the user to wipe the environment, but I'm not sure the best way of doing it, a new user probably doesn't know what wipe the environment is

If the user doesn't know, he/she can go to our documentation or finally fill an issue and somebody will help them.

As Eric said, don't worry too much about this for now. We will find a better UX later.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Jun 18, 2018

I removed the "fix" and add a note to test this later, also I opened #4258 and #4259

@ericholscher ericholscher merged commit 396a84e into readthedocs:master Jun 19, 2018
1 check passed
@ericholscher
Copy link
Member

@ericholscher ericholscher commented Jun 19, 2018

🎆

@stsewd stsewd deleted the stuck-stable branch Jun 19, 2018
@jaraco
Copy link
Contributor

@jaraco jaraco commented Jun 19, 2018

Wow. That was a lot of work. Thanks so much.

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