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

Refactoring and API #505

Merged
merged 196 commits into from
Jan 28, 2019
Merged

Refactoring and API #505

merged 196 commits into from
Jan 28, 2019

Conversation

encukou
Copy link
Member

@encukou encukou commented Dec 30, 2018

Here's a start for #494.
I recommend reading the full diff instead of the individual commits. Squash when ready.

If you'd like to review this, let me know in the comments, so I don't merge without your input.

Features/Changes

  • A JSON API with two flavors: input (what we get from "forks") and output (available at at /v0/naucse.json)
    • The API is described and validated using jsonschema.
    • Uses HTML for content, never Markdown.
    • Relative URLs use a custom naucse: schema for the API. This gets converted in the HTML "sanitization" step.
  • The part that should be in "forks" is moved to the naucse_render module, which should become its own repo & PyPI package.
  • When one course's build fails, everything fails. No silent "an error occured" pages.
    • Only fully applies in "freeze mode" (with NAUCSE_FREEZE=1, or Flask in non-debug mode).
    • In debug mode, lessons are rendered on demand instead of at startup.
  • Doesn't do fancy guessing of repo URL&branch for edit info. Use NAUCSE_MAIN_REPO_URL and NAUCSE_MAIN_REPO_BRANCH if needed (e.g. on Travis).
  • The model is better defined. Notably:
    • A course has a list of sessions
    • A session has a list of materials
    • A material may be for a lesson (or an external link)
    • A course (!) has lessons (which are referenced by materials). (All lessons are in a course; there's a special hidden lessons course containing the "local"/"canonical" lessons.)
  • The distinction between forks and local content is disappearing – I'd like to get rid of local content eventually.
  • The distinction between "courses" and "runs" in the model is disappearing: mostly they just have different slugs (e.g. courses/mi-pyt vs. 2018/pyladies-brno-jaro) and are in different collections for lookup.
  • Instead of FORKS_ENABLED=true, there's NAUCSE_TRUSTED_REPOS='*'. You can filter allowed repos a bit (docs TODO).
  • The model is different (converters.py is utilities for stuff you can load/dump from/to a versioned JSON API).
  • Pages request LaTeX using "modules: {'katex': '0.7.1'}. This should become a general mechanism to request some extra functionality (like JS/CSS).
  • Model objects have info about their URLs.

Major Hacks

Injects code into arca forks

Currently, forks of naucse don't export structured data; almost everything is converted to HTML on the fork side. This makes getting e.g. a list of sessions quite hard. This PR adds a nasty hack that injects code into the container (in arcs_renderer.py).
My plan is like to install naucse_render=0.1 into the Arca containers instead of injecting the code. Looking for better solutions.

API v0

The API is at not finalized, but should be useful. It's "version 0", i.e. please coordinate if you use it.

naucse_render

I intend to use naucse_render == 0.1 as a one-off for "legacy" forks. Once there's an API, this can be beautified together with the source data.

Ugly templates

I kept changes in the result minimal, but did refactor templates a bit. Whitespace/indentation looks weird sometimes.

TODO

  • Unit tests
    • Fix the xfail
    • Remove test_forks.py. (I'd like to go through it once more before that).
  • Docs (i.e. updating the README and meta course).
  • Split out naucse_render

How to test

Get a baseline from "almost master".

On a VM:

$ git checkout sanitize-absolute-urls
$ ARCA_BACKEND=arca.backend.DockerBackend FORKS_ENABLED=true ARCA_BACKEND_REGISTRY_PULL_ONLY=1 TRAVIS=1 TRAVIS_REPO_SLUG=pyvec/naucse.python.cz TRAVIS_BRANCH=master NAUCSE_CALENDAR_DTSTAMP=2018-11-22 pipenv run naucse freeze -v
$ mv naucse/_build/ _build_master

Clean the caches

just to be sure:

$ rm -rfv .arca/
$ docker stop $(docker ps -a -q) && docker container prune -f

Freeze

$ git checkout fork-api
$ NAUCSE_MAIN_REPO_URL=https://github.com/pyvec/naucse.python.cz/ NAUCSE_CALENDAR_DTSTAMP=2018-11-22 NAUCSE_TRUSTED_REPOS='*' ARCA_BACKEND=arca.backend.DockerBackend ARCA_BACKEND_REGISTRY_PULL_ONLY=1 pipenv run naucse freeze -v 

Compare

$ YOUR_FAVORITE_RECURSIVE_DIFF_TOOL="diff -r -U3"
$ $YOUR_FAVORITE_RECURSIVE_DIFF_TOOL _build_master/ naucse/_build

For me, the unified diff has 4649 lines :(
But it's mostly trivial differences – in things that shouldn't be rendered in forks, but were.

@encukou
Copy link
Member Author

encukou commented Jan 18, 2019

The Travis freeze failure says: ModuleNotFoundError: No module named 'naucse_render'

That goes from a fork i guess, but how can I see which one?

The traceback now has a wrapper error with info on the repo and method called.
I went and fixed that issue, a typo in the version, without review.

A fork wants a newer version of our API that we don't yet have. We check that the error mentions that.

Versions are ignored entirely for now; there's no error and no bad input to check.

The command is gone.

The main reason for this workaround was that without output, Travis CI
would think the process stalled.
More output/logging overall and `elsa freeze -v` should ensure there's
enough output.
@hroncok
Copy link
Member

hroncok commented Jan 18, 2019

typo in the version, without review.

actually my bad review :( but at least we see it works :)

encukou and others added 7 commits January 18, 2019 22:31
Previously, only runs could be loaded from link.yml.
This limitation doesn't make much sense.
Co-Authored-By: encukou <encukou@gmail.com>
This makes the test a bit more thorough, as it includes the whitelisted
`#master` (but does not end with it).
@encukou
Copy link
Member Author

encukou commented Jan 18, 2019

Optionally unittest converters but that should be covered by some of high level tests above so this can remain as an open issue for "next" time.

Yup. Filed as https://github.com/pyvec/naucse.python.cz/issues/509.

  • A fork is made, yet the fork doesn't have naucse_renderer in Pipfile. The exception mentions that.

For some reason, I can't seem to get the right error in this case :/
Hopefully that's the last issue for tests. Remaining:

  • Docs (i.e. updating the README and meta course).
  • Remove test_forks.py. (I'd like to go through it once more before that).

Anything else you can think of?

Signing out for today.

@hroncok
Copy link
Member

hroncok commented Jan 18, 2019

For some reason, I can't seem to get the right error in this case :/

what error you get? the travis log from above shows what happens, doesn't it?

Hopefully that's the last issue for tests. Docs are still remaining (i.e. updating the README and meta course). Anything else you can think of?

Good work with the tests, thank you. Any point of keeping the skipped test_forks? Do you have plans with them?

I must admit that I wasn't able to review everything so thoroughly as I'd like, but that would take me a couple weeks. For others: I've consulted this PR with @encukou in person as well.

One problem I have is that everything is soooo complicated at this point that debugging any problem has a bus factor of 3 (@encukou @mikicz and me) and after this, @mikicz might no longer be counted into this. We shall not travel with the same train from now on 😁

I am afraid that at this point we are too deep in the rabbit hole and this PR doesn't make it that worse. Hopefully it allows us to make it better. Thanks for that.

Signing out for today.

Good night 💤

@encukou
Copy link
Member Author

encukou commented Jan 19, 2019

For some reason, I can't seem to get the right error in this case :/

what error you get? the travis log from above shows what happens, doesn't it?

No error. If I delete the Pipfile/Pipfile.lock, and put in a requirements.txt without naucse_render, everything seems to work. And it's all hidden somewhere in Arca :(
Will look into it, but I need to do DevConf preparations this weekend.

Hopefully that's the last issue for tests. Docs are still remaining (i.e. updating the README and meta course). Anything else you can think of?

Good work with the tests, thank you. Any point of keeping the skipped test_forks? Do you have plans with them?

I'd like to go through it once more before removing it. Possibly after this PR is merged.

I must admit that I wasn't able to review everything so thoroughly as I'd like, but that would take me a couple weeks. For others: I've consulted this PR with @encukou in person as well.

One problem I have is that everything is soooo complicated at this point that debugging any problem has a bus factor of 3 (@encukou @mikicz and me) and after this, @mikicz might no longer be counted into this. We shall not travel with the same train from now on grin

I am afraid that at this point we are too deep in the rabbit hole and this PR doesn't make it that worse. Hopefully it allows us to make it better. Thanks for that.

Indeed, that's the goal here. Not so much to reduce the overall complexity, but to separate it into individual pieces, so someone can focus on one thing (in naucse or outside), and ignore the others.

@hroncok
Copy link
Member

hroncok commented Jan 19, 2019

BTW I couldn't sleep last night and was thinking about our git tests that we currently somehow subprocess in elsa, here and I'll possibly also need that when I'll work on a git merge driver for spec files. My idea was to create a common pytest-git plugin that would abstract this to some utility git workspace fixture. I even started to design it in my head. This morning I've decided to draft that thing, but apparently, it was obviously already done and called the same :D

https://pypi.org/project/pytest-git/

I'm not saying let's start using it now when the tests are already written, I'm just pointing it out for future reference. I will play with that thing when I test the merge driver to see how useful it is.

@mikicz
Copy link
Member

mikicz commented Jan 19, 2019

For some reason, I can't seem to get the right error in this case :/

what error you get? the travis log from above shows what happens, doesn't it?

No error. If I delete the Pipfile/Pipfile.lock, and put in a requirements.txt without naucse_render, everything seems to work. And it's all hidden somewhere in Arca :(
Will look into it, but I need to do DevConf preparations this weekend.

I definitely can't read the whole dff/conversation, but I noticed this bit in a notification and I think I could take a short look if there's a problem with something in Arca or with Arca? Am I understanding correctly, that when you use the same fork with and without Pipfile/Pipfile.lock it produces a different result?

mikicz and others added 2 commits January 27, 2019 23:39
@encukou
Copy link
Member Author

encukou commented Jan 27, 2019

I definitely can't read the whole dff/conversation, but I noticed this bit in a notification and I think I could take a short look if there's a problem with something in Arca or with Arca? Am I understanding correctly, that when you use the same fork with and without Pipfile/Pipfile.lock it produces a different result?

Sorry for the delay.

I've added the failing test in 1162e65
Hopefully it's clear enough. There are a few variations I've used, but I'm not sure how to make it fail.

@encukou
Copy link
Member Author

encukou commented Jan 27, 2019

The API is blocking two external projects (coursweare and feedbacker).
Let us merge, and put remaining work (tests, docs) in issues. The whole thing is a big integration test – and that passes.

@encukou encukou merged commit 29ab964 into pyvec:master Jan 28, 2019
@encukou
Copy link
Member Author

encukou commented Jan 28, 2019

It's alive! Thanks everyone for your help!

TODO items merged into #494

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.

4 participants