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

Update mkdocs #5505

Merged
merged 3 commits into from Apr 2, 2019
Merged

Update mkdocs #5505

merged 3 commits into from Apr 2, 2019

Conversation

@stsewd
Copy link
Member

@stsewd stsewd commented Mar 19, 2019

Things I'm seeing so far:

  • The edit/view link is wrong (with the current version actually)
  • Previous theme (readthedocs) isn't used
  • The versions list is covered by the header of the mkdocs theme when there are a lot of versions (this is a problem with our z-index)

Screenshot_2019-03-19 Writing Your Docs - MkDocs

Closes #5332

  1. is a problem already present, we can fix that in another PR
  2. I think this is what we want, but I can deep a little more if we are correctly editing the mkdocs.yml file
  3. This is only present when the user has a lot of versions, we can fix it later or in another PR I think
@stsewd
Copy link
Member Author

@stsewd stsewd commented Mar 19, 2019

We are still fine defaulting to

https://github.com/rtfd/readthedocs.org/blob/719f3ca7752c66a0fd3d89043eec2a62dae430de/readthedocs/doc_builder/backends/mkdocs.py#L92-L94

As a minimum this configuration file must contain the site_name setting. All other settings are optional.

https://mkdocs.readthedocs.io/en/latest/user-guide/configuration/

@stsewd
Copy link
Member Author

@stsewd stsewd commented Mar 19, 2019

docs_dir is still valid

The directory containing the documentation source markdown files. This can either be a relative directory, in which case it is resolved relative to the directory containing your configuration file, or it can be an absolute directory path from the root of your local file system. default: 'docs'

@stsewd
Copy link
Member Author

@stsewd stsewd commented Mar 19, 2019

@stsewd
Copy link
Member Author

@stsewd stsewd commented Mar 19, 2019

The theme problem was correct actually

https://github.com/rtfd/readthedocs.org/blob/a639939b77b3ec803abb8dcf749a1d6d7d9d8254/readthedocs/doc_builder/backends/mkdocs.py#L182-L187

I think we put that feature flag only to old projects, right? If so, we can just leave that code as is, new projects should set explicitly the readthedocs theme.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Mar 19, 2019

From the floating footer, our z-index is 400, the z-index from the mkdocs header is 1030. That's the problem.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Mar 19, 2019

Our z-index comes from https://github.com/rtfd/readthedocs.org/blob/master/readthedocs/core/static/core/css/badge_only.css, not sure from where we generate that min file.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Mar 19, 2019

Got it, it comes from https://github.com/rtfd/readthedocs.org/blob/a639939b77b3ec803abb8dcf749a1d6d7d9d8254/gulpfile.js#L27-L27

Whici comes from https://github.com/rtfd/readthedocs.org/blob/a639939b77b3ec803abb8dcf749a1d6d7d9d8254/bower.json#L20-L20

We should update the theme in the dependencies, I guess? Or do we want to follow another approach for mkdocs?

@stsewd stsewd requested a review from Mar 19, 2019
@@ -295,7 +291,7 @@ def install_core_requirements(self):
]

if self.config.doctype == 'mkdocs':
requirements.append('mkdocs==0.17.3')
requirements.append('mkdocs<1.1')
Copy link
Member Author

@stsewd stsewd Mar 19, 2019

I haven't seen anything breaking from their changelog since 1.0 :)

https://mkdocs.readthedocs.io/en/latest/about/release-notes/

1.1 is in current development

@stsewd
Copy link
Member Author

@stsewd stsewd commented Mar 19, 2019

@davidfischer
Copy link
Contributor

@davidfischer davidfischer commented Mar 20, 2019

not sure from where we generate that min file.

I believe it comes from the RTD sphinx theme. However, the badge_only.css isn't the full sphinx theme CSS. It is only for the flyout menu.

I think we put that feature flag only to old projects, right? If so, we can just leave that code as is, new projects should set explicitly the readthedocs theme.

The big issue is that users can pin to 0.17 or 1.0 in the requirements.txt file. When they do, we have to be very careful of any changes we make to the mkdocs.yml file (the fewer the better, IMO) since many aren't compatible between versions.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Mar 20, 2019

The big issue is that users can pin to 0.17 or 1.0 in the requirements.txt file. When they do, we have to be very careful of any changes we make to the mkdocs.yml file (the fewer the better, IMO) since many aren't compatible between versions.

I get it, the issue is with users with the feature flag and without a pinned version of mkdocs, their projects will fail or have the other theme, I'm not sure how new versions of mkdocs handled that deprecated setting.

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Mar 22, 2019

@stsewd not sure what the status here is? I imagine users can already install this version via requirements, so we mostly just need to make sure it doesn't break horribly for everyone when we upgrade. I guess if users aren't pinning it currently, they might get invalid config errors from the upgraded version? Not really anything we can do about that, sadly :/

@stsewd
Copy link
Member Author

@stsewd stsewd commented Mar 25, 2019

I guess if users aren't pinning it currently, they might get invalid config errors from the upgraded version? Not really anything we can do about that, sadly :/

Yeah, the only affected users are the ones with the flag, I think we put that flag on all the projects of .com.

So if the projects have the flag, and they don't have the theme option set, they will not be able to use a newer version of mkdocs/their build will fail.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Mar 25, 2019

I don't think we used the flag in .org, so here we are safe.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Mar 27, 2019

Also, the current mkdocs version that we install doesn't work with python 3.7 (the default version that we install)

Copy link
Member

@humitos humitos left a comment

I'm 👍 on upgrading our default installed version to the latest available at this point. Although, I think we need to be very careful here with existing projects.

I'd like to put this under a feature flag, so current projects keep using 0.17.3 if they don't have a pinned version and new projects start using <1.1. Otherwise, I won't merge this PR because I'm mostly sure that it will break existent projects randomly and will make this difficult to debug.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Mar 27, 2019

Putting a feature flag for old projects makes sense, but it also makes the feature flag to be around for ever, projects will break if they don't pin their requirements or keep updating their docs the latest version of mkdocs supported. Same for sphinx, we update sphinx quicker, that's why we don't have many users yelling at us, they update progressively.

@humitos
Copy link
Member

@humitos humitos commented Mar 27, 2019

Putting a feature flag for old projects makes sense, but it also makes the feature flag to be around for ever,

Unfortunately, yes. We need the feature flag until we decide to drop support for old mkdocs versions.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Mar 27, 2019

Well, this change doesn't mean to stop supporting old mkdocs versions, projects should still work, only projects that have a mkdocs.yaml file with incompatible settings in 1.0 and without a pinned version will fail.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Apr 1, 2019

Feature flag was added, how do we manage this? Do we create the feature flag with deafault_true=True in a migration or from the admin?

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Apr 2, 2019

Do we create the feature flag with deafault_true=True in a migration or from the admin?

Not sure if we create a migration, or just make that setting True when we create it in prod. A migration feels like the best option, so it doesn't depend on a step during deploy other than running migrations.

@humitos
Copy link
Member

@humitos humitos commented Apr 2, 2019

We have been creating the feature flag when deploying as far as I know. We will need to do this twice: community and corporate, though.

humitos
humitos approved these changes Apr 2, 2019
Copy link
Member

@humitos humitos left a comment

This is 👍 now that it's under a feature flag.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Apr 2, 2019

I was thinking, creating the feature flag with now (at migration time) looks weird (for new instances of rtd). Putting an exact date of the deploy is kind of guessing and not exactly at the same time of the webs (and it will be weird for new instances of rtd).. So, I'm +1 to create it after the deploy.

@stsewd stsewd merged commit ba370fc into readthedocs:master Apr 2, 2019
1 check passed
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.

4 participants