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

Projects
None yet
5 participants
@stsewd
Member

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

This comment has been minimized.

Member

stsewd commented Apr 5, 2018

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

@stsewd

This comment has been minimized.

Member

stsewd commented Apr 5, 2018

I saw that there is a test case that allows the user to re-define the stable version

def test_user_defined_stable_version_with_tags(self):

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

@RichardLitt RichardLitt changed the title from Stuck stable to `stable` version stuch on a specific commit Apr 9, 2018

@RichardLitt RichardLitt changed the title from `stable` version stuch on a specific commit to `stable` version stuck on a specific commit Apr 9, 2018

)
self.assertEqual(resp.status_code, 200)
self.pip.refresh_from_db()

This comment has been minimized.

@stsewd

stsewd Apr 10, 2018

Member

I'm not sure if this is necessary here.

This comment has been minimized.

@humitos

humitos Apr 10, 2018

Member

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.

This comment has been minimized.

@humitos

humitos Apr 10, 2018

Member

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')

This comment has been minimized.

@stsewd

stsewd Apr 10, 2018

Member

The version can collide with another fixture project

@stsewd

This comment has been minimized.

Member

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):

This comment has been minimized.

@stsewd

stsewd Apr 10, 2018

Member

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

This comment has been minimized.

@humitos

humitos Apr 10, 2018

Member

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.

This comment has been minimized.

@stsewd

stsewd Apr 10, 2018

Member

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

default = project.default_branch or (project.vcs_repo().fallback_branch)
if not project.has_valid_webhook:
project.has_valid_webhook = True
project.save()
if slug == default and slug not in already_built:
# short circuit versions that are default
# these will build at "latest", and thus won't be
# active
latest_version = project.versions.get(slug=LATEST)

# latest is a special case where we don't have to check if it exists
if self.default_version == LATEST:
return self.default_version
# check if the default_version exists
version_qs = self.versions.filter(
slug=self.default_version, active=True
)
if version_qs.exists():
return self.default_version
return LATEST

# LATEST is special as it is usually a branch but does not contain the
# name in verbose_name.
if self.slug == LATEST:
if self.project.default_branch:
return self.project.default_branch
return self.project.vcs_repo().fallback_branch

if self.default_branch:
latest = self.versions.get(slug=LATEST)
if latest.identifier != self.default_branch:
latest.identifier = self.default_branch
latest.save()

branch = self.default_branch or self.vcs_repo().fallback_branch
if not self.versions.filter(slug=LATEST).exists():
self.versions.create_latest(identifier=branch)

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.

This comment has been minimized.

@humitos

humitos Apr 16, 2018

Member

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?

This comment has been minimized.

@stsewd

stsewd Apr 16, 2018

Member

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

if self.default_branch:
latest = self.versions.get(slug=LATEST)
if latest.identifier != self.default_branch:
latest.identifier = self.default_branch
latest.save()

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):

This comment has been minimized.

@humitos

humitos Apr 16, 2018

Member

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()

This comment has been minimized.

@stsewd

stsewd Apr 17, 2018

Member

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).

This comment has been minimized.

@humitos

humitos Apr 17, 2018

Member

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

This comment has been minimized.

@stsewd

stsewd Apr 17, 2018

Member

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

@humitos

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()

This comment has been minimized.

@humitos

humitos Apr 17, 2018

Member

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

This comment has been minimized.

@stsewd

stsewd Apr 17, 2018

Member

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

This comment has been minimized.

@humitos

humitos Apr 17, 2018

Member

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.

This comment has been minimized.

@stsewd

stsewd Apr 17, 2018

Member

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

This comment has been minimized.

@humitos

humitos Apr 17, 2018

Member

Yes. Is is used inside the loop?

This comment has been minimized.

@stsewd

stsewd Apr 18, 2018

Member

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

This comment has been minimized.

@humitos

humitos Apr 17, 2018

Member

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.

This comment has been minimized.

@humitos

humitos Apr 17, 2018

Member

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

self.assertEqual(
current_stable.identifier,
version_stable.identifier
)

This comment has been minimized.

@humitos

humitos Apr 17, 2018

Member

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.

This comment has been minimized.

@humitos

humitos Apr 17, 2018

Member

and the current_stable.identifier should be 1abc2def3

This comment has been minimized.

@stsewd

stsewd Apr 17, 2018

Member

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

This comment has been minimized.

@stsewd

stsewd Apr 18, 2018

Member

The tests from TestStableVersion already check that

version_stable = self.pip.versions.get(slug='stable')
self.assertFalse(version_stable.machine)
self.assertTrue(version_stable.active)
self.assertEqual(
'1abc2def3',
self.pip.get_stable_version().identifier
)

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

This comment has been minimized.

@humitos

humitos Apr 17, 2018

Member

Same here.

self.assertEqual(
current_stable.identifier,
version_stable.identifier
)

This comment has been minimized.

@humitos

humitos Apr 17, 2018

Member

Check for machine status here and use hardcoded identifier.

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

This comment has been minimized.

@humitos

humitos Apr 17, 2018

Member

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):

This comment has been minimized.

@humitos

humitos Apr 17, 2018

Member

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

This comment has been minimized.

@stsewd

stsewd Apr 17, 2018

Member

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(

This comment has been minimized.

@humitos

humitos Apr 17, 2018

Member

other_stable?

This comment has been minimized.

@stsewd

stsewd Apr 17, 2018

Member

Ups, copy paste

@stsewd

This comment has been minimized.

Member

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

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

@humitos

This comment has been minimized.

Member

humitos commented May 1, 2018

@stsewd I know there were different unrelated issues with this PR that you were fixing in the process, thank! Besides that, were you able to make some progress on this PR? I'm interesting on having this merged sooner than later.

@stsewd

This comment has been minimized.

Member

stsewd commented May 1, 2018

@humitos I'll try to continue this today by applying my current fix for the issue locally only.

@ericholscher

This comment has been minimized.

Member

ericholscher commented Jun 12, 2018

Hmm, seems like this is conflicting, but looks good to me once that's fixed! 👍

@humitos

This comment has been minimized.

Member

humitos commented Jun 12, 2018

Can we just raise an error if a repo has both a stable or latest tag and branch?

I supose, yes.

My first position was to solve these edge cases with Python code without involving the user since we already known the problems and the solutions, but then Santos explained it to me and it's not that easy because we need to change a lot of RTD code which will produce unexpected behaviour.

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.

@humitos

The logic looks good to me.

I left some comments that I'd like you to consider and adapt the code to them.

@@ -43,6 +43,10 @@ class RepositoryError(BuildEnvironmentError):
'One or more submodule URLs are not valid.'
)
DUPLICACATE_RESERVED_VERSIONS = _(

This comment has been minimized.

@humitos

humitos Jun 12, 2018

Member

There is a typo in DUPLICACATE_ word :)

This comment has been minimized.

@stsewd

stsewd Jun 12, 2018

Member

There is a duplicate letter in duplicate 😆

@@ -151,6 +152,18 @@ def sync_repo(self):
except Exception:
log.exception('Unknown Sync Versions Exception')
def validate_duplicate_reserved_versions(self, data):

This comment has been minimized.

@humitos

humitos Jun 12, 2018

Member

I'd like to see a docstring here ;)

@@ -123,3 +130,56 @@ def test_sync_repository(self):
args=(version.pk,),
)
self.assertTrue(result.successful())
@patch('readthedocs.projects.tasks.api_v2')
def test_check_duplicate_reserved_version_latest(self, api_v2):

This comment has been minimized.

@humitos

humitos Jun 12, 2018

Member

I think this one and the next one are similar enough to use pytest.mark.parametrize with stable and latest as arguments: https://docs.pytest.org/en/latest/parametrize.html

Also, instead of stable and latest use the LATEST and STABLE constants.

This comment has been minimized.

@stsewd

This comment has been minimized.

@humitos

humitos Jun 12, 2018

Member

:(

What about creating a _helper method with the duplicated code and call it twice from the test?

command = ['git', 'tag', '-delete', tag]
check_output(command, env=env)
chdir(path)

This comment has been minimized.

@humitos

humitos Jun 12, 2018

Member

Instead of saving the path and recovering it before the functions returns, we should use this decorator:

def restoring_chdir(fn):
# XXX:dc: This would be better off in a neutral module
@wraps(fn)
def decorator(*args, **kw):
try:
path = os.getcwd()
return fn(*args, **kw)
finally:
os.chdir(path)
return decorator

@stsewd

This comment has been minimized.

Member

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

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 rtfd:master Jun 19, 2018

1 check passed

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

This comment has been minimized.

Member

ericholscher commented Jun 19, 2018

🎆

@stsewd stsewd deleted the stsewd:stuck-stable branch Jun 19, 2018

@jaraco

This comment has been minimized.

Contributor

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