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

Don't build latest on webhook if it is deactivated #4733

Merged
merged 10 commits into from Nov 1, 2018

Conversation

@stsewd
Copy link
Member

@stsewd stsewd commented Oct 4, 2018

Fixes #4672

@@ -42,16 +42,26 @@ def _build_version(project, slug, already_built=()):
# these will build at "latest", and thus won't be
# active
Copy link
Member Author

@stsewd stsewd Oct 4, 2018

Not really sure if this short circuit is saving resources, it only saves one query (probably). I think this piece only adds complexity to the code. Let me know if you want me to remove this.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Oct 4, 2018

This is weird... for some reason the test failed only on python2, I tested it locally with python3, the test fail there too.

@stsewd stsewd requested a review from Oct 4, 2018
@stsewd
Copy link
Member Author

@stsewd stsewd commented Oct 5, 2018

If someone is reviewing this, please test locally 9bec7e0 with python3

.filter(slug=slug)
.exists()
)
if version_exists:
Copy link
Member

@ericholscher ericholscher Oct 5, 2018

Is there a reason we want to do this check only when the latest version is active? I'm not 100% sure, but it seems like we'd want it to run regardless of the state of the latest version.

Copy link
Member Author

@stsewd stsewd Oct 5, 2018

A user deactivates latest, here we try to build latest when the branch is master, as a side effect the version gets activated.

I just realized that we may need this strange logic, as this allows us to update the version list... Maybe we want to merge this after #4450 ?

Copy link
Member

@humitos humitos Oct 10, 2018

I'm not sure to follow @stsewd. If the branch is master we will have two versions: master and latest.

When a webhook for master is received, we will want to build both if they are enabled. If latest is disabled we will want to build master version. Right?

Copy link
Member Author

@stsewd stsewd Oct 10, 2018

Yup, master is still building here, the code above bellow handle that case, that's why I'm suggesting to remove this block too, as I think it only adds complexity to the code. But without building latest (activated or not) we don't have a way of updating the branch list, users will need to trigger a build to any other version to get the latest tags/branches on rtd.

Copy link
Member

@humitos humitos Oct 11, 2018

But without building latest (activated or not) we don't have a way of updating the branch list, users will need to trigger a build to any other version to get the latest tags/branches on rtd.

A similar case is already happening in RTD:

  1. import your project
  2. create a new branch in your repository
  3. go to Versions tab in RTD

Your new branch is not there.

I think we should implement a completely separate way of syncing our version with the ones on the repo. I mean, we should not build anything if we receive a webhook, but update the versions list first. Then, decide what we need to build and trigger build those versions.

(this is probably out of the scope of this PR, but maybe worth thinking a little about this here)

that's why I'm suggesting to remove this block too, as I think it only adds complexity to the code

This is the part where I don't follow you :(

Copy link
Member Author

@stsewd stsewd Oct 11, 2018

#4450 this issue is trying to solve the sync problem, so, maybe is worth to solve that first? What do you think?

This is the part where I don't follow you :(

This line just check if we are receiving a main branch

https://github.com/stsewd/readthedocs.org/blob/76da8e5330c0523e6acc61ea561b3fb0b733e3d4/readthedocs/core/views/hooks.py#L40-L40

That way we build latest and the main branch at the same time. For the rest of versions, this is the normal flow

https://github.com/stsewd/readthedocs.org/blob/76da8e5330c0523e6acc61ea561b3fb0b733e3d4/readthedocs/core/views/hooks.py#L70-L79

So, at the end we are just saving one loop, even we aren't saving queries to the db now

Copy link
Member

@humitos humitos Oct 12, 2018

#4450 this issue is trying to solve the sync problem, so, maybe is worth to solve that first? What do you think?

I'm fine of merging this PR first if it doesn't introduce another issue/bug/wrong workflow/confuse users.

#4450 is already a problem, and this PR doesn't fix it. Which is fine to me. I don't want to fix that issue in this PR, but I want this statement to be correct to merge:

When a webhook for master is received, we will want to build both if they are enabled. If latest is disabled we will want to build master version.

Anyway, I'm still confused with this chunk of code and it seems that talking async doesn't clarify things. We should chat/talk in sync mode.

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Oct 5, 2018

rtd_tests/tests/test_api.py .                                                                                  [100%]

===================================== 1 passed, 1048 deselected in 9.27 seconds ======================================
______________________________________________________ summary _______________________________________________________
  py36: commands succeeded
  congratulations :)

Seemed to work here on your latest branch.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Oct 5, 2018

@ericholscher with only the test?

Copy link
Member

@humitos humitos left a comment

Changes make sense to me, but I'd like to continue the discussion before merge:

  • we should log that we are not building latest when it's disabled.
  • I think we should build the master branch even if latest is disabled.

If we arrive at the same conclusion there, I'm happy to merge.

.filter(slug=slug)
.exists()
)
if version_exists:
Copy link
Member

@humitos humitos Oct 10, 2018

I'm not sure to follow @stsewd. If the branch is master we will have two versions: master and latest.

When a webhook for master is received, we will want to build both if they are enabled. If latest is disabled we will want to build master version. Right?

"(Version build) Building %s:%s",
project.slug, slug_version.slug
)
return LATEST

if project.versions.exclude(active=True).filter(slug=slug).exists():
Copy link
Member

@humitos humitos Oct 10, 2018

Isn't this query easily re-written as follow?

project.versions.filter(active=False, slug=slug).exists()

I found it way easier to read it.

log.info("(Version build) Building %s:%s",
project.slug, slug_version.slug)
return LATEST
if latest_version.active:
Copy link
Member

@humitos humitos Oct 10, 2018

The case where latest is disabled is missing.

Example:

if latest.enabled:
  log.info('Building latest')  # note that it's important to log first then trigger
  trigger_build
else:
  log.info('Not building latest because disabled')
  return None

Copy link
Member Author

@stsewd stsewd Oct 10, 2018

note that it's important to log first then trigger

I wanna to change that, but I wasn't sure if that was on purpose 🤔

Copy link
Member Author

@stsewd stsewd Oct 11, 2018

The other logs do that too btw, so maybe we want to change those too

Copy link
Member

@humitos humitos Oct 11, 2018

Go ahead!

The idea of logging first is that if the following line fail for any reason, we miss the log and we don't know "where" it could have fail.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Oct 15, 2018

I removed the short circuit, added more tests and left a comment linking to #4450

Copy link
Member

@humitos humitos left a comment

¡Me encanta!

Left a comment about a test that I'd like to have. If we already have that test, this is ready to merge to me.

Also, if you didn't do it yet, please do a small QA in your local environment to be sure that this will keep working.

default_branch = self.project.versions.get(slug='master')

self.assertTrue(latest_version.active)
self.assertTrue(default_branch.active)
Copy link
Member

@humitos humitos Oct 15, 2018

Can you add another test (if it's not already) that test when latest is inactive but not the default_branch that the latest version is not triggered for build an is not re-activated and the default_branch is triggered?

@stsewd
Copy link
Member Author

@stsewd stsewd commented Oct 15, 2018

I have tested this locally, it works. I'm not sure how to test the re-activate thing, that is a side effect of the build process (and we mock the trigger_build function here), so I have added an assert to the mock.

@humitos
Copy link
Member

@humitos humitos commented Oct 16, 2018

Mhr... That makes me thing "Where do we automatically mark the triggered version to be built as active?". I grepped the code, and I only find this line:

https://github.com/rtfd/readthedocs.org/blob/6e22661060bea2b3fe388c744b5755d78963edb1/readthedocs/restapi/views/model_views.py#L220

but that one is only for the stable version, I think.

@stsewd do you know where that is happening? That's the chunk of code that we need a test for.

@humitos
Copy link
Member

@humitos humitos commented Oct 16, 2018

The other place I found something related is

https://github.com/rtfd/readthedocs.org/blob/6e22661060bea2b3fe388c744b5755d78963edb1/readthedocs/restapi/views/model_views.py#L113-L115

but I think it's not the place where our problem is happening.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Oct 16, 2018

I'm not sure where it's happening, but maybe that's by design or something else. Either way, not sure if we can add a test for that here, it's triggered inside the mocked method, we should test that in another part, but we will testing that it activates when building (if that is by design) p:

@humitos
Copy link
Member

@humitos humitos commented Oct 16, 2018

I'm not sure where it's happening, but maybe that's by design or something else

Wait. If we don't know where this is happening, how are we sure that this PR will fix this issue of not re-activating the latest version?

Also, here #4733 (comment), you mentioned this exact behavior --now, I suppose that you knew this because of manual QA. I think this need a test because looks like a black box :)

Anyway, I suppose that we can merge this PR but I'd prefer if you open a new issue for adding a test on this scenario. Looks tricky and we don't really know if this will still be present in production after merging this PR or not.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Oct 16, 2018

It happens on trigger_build, we have tests that check that the function wasn't called.

@humitos
Copy link
Member

@humitos humitos commented Oct 16, 2018

It happens on trigger_build, we have tests that check that the function wasn't called.

Where? Can you point me the specific line? I didn't find it.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Oct 16, 2018

I mean, it happens inside that, and we check trigger_build to not be called

@stsewd
Copy link
Member Author

@stsewd stsewd commented Oct 22, 2018

I opened #4793

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Oct 23, 2018

Looks like everyone is okay with this PR. As I understand it, this PR is just changing the webhook behavior, it doesn't solve the underlying issue that latest is being reactivated with some magic somewhere (perhaps the patch call you pointed out in #4793)?

@stsewd
Copy link
Member Author

@stsewd stsewd commented Oct 23, 2018

The version isn't reactivated here, as it doesn't hit trigger_build, the other issue is just for further lookup. I tested this locally, and if latest is deactivated, then isn't built and remains deactivated.

@humitos
Copy link
Member

@humitos humitos commented Nov 1, 2018

I'm 👍, again, on merging this PR :)

Copy link
Member

@ericholscher ericholscher left a comment

Tests look like the behavior we want. 👍

@ericholscher ericholscher merged commit 5f3d11e into readthedocs:master Nov 1, 2018
1 check passed
@stsewd stsewd deleted the fix-webhook-latest branch Nov 1, 2018
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

4 participants