Skip to content
This repository has been archived by the owner on Mar 18, 2022. It is now read-only.

Add submodule configuration #30

Closed
5 tasks
agjohnson opened this issue Nov 2, 2017 · 50 comments
Closed
5 tasks

Add submodule configuration #30

agjohnson opened this issue Nov 2, 2017 · 50 comments
Labels
Accepted Accepted issue on our roadmap Feature New feature

Comments

@agjohnson
Copy link
Contributor

agjohnson commented Nov 2, 2017

Seems a number of projects are hitting issues with submodule clone steps, and this should be configurable in our config. Because git doesn't provide the mechanism for optional submodules, we can do something like:

submodules:
  include: []
  recursive: false

as implicit default. And:

submodules:
  include:
    - foo/
    - bar/

As include example.

submodules:
  exclude:
    - foo/

^ is the exclude example.

Legacy projects implemented before this change would have an implicit configuration of:

submodules:
  include: all
  recursive: true

It's not possible to include and exclude submodules, this is a validation error.

This would eventually alter the code that rtfd/readthedocs.org uses for submodule cloning, and would only selectively clone things? The changes there are likely moving to specific submodule clone commands.

  • Currently submodule cloning takes place in the git checkout step. This will need to be moved to a new step after we parse the configuration
  • New behavior will be to not clone submodules automatically
  • If possible, we won't recursively clone as well. Maybe this is another option on the spec?
  • We'll add a new project.Feature, plus migration, to lock old users into the old behavior (all submodules are cloned)
  • We can perhaps also detect that a project has submodules that won't be cloned and raise a warning. This warning shouldn't be a build failure, but a notification on-site/through email?
@haubourg
Copy link

+1 we hit that currently and this is a blocker to us

agjohnson added a commit to readthedocs/readthedocs.org that referenced this issue Mar 20, 2018
This adds a project feature that allows for a project to specify that they would
like to skip submodule installation. Currently we are forcing all submodules to
be checked out, so this fails on private submodules.

Refs readthedocs/readthedocs-build#30
agjohnson added a commit to readthedocs/readthedocs.org that referenced this issue Mar 20, 2018
This adds a project feature that allows for a project to specify that they would
like to skip submodule installation. Currently we are forcing all submodules to
be checked out, so this fails on private submodules.

Refs readthedocs/readthedocs-build#30
agjohnson added a commit to readthedocs/readthedocs.org that referenced this issue Mar 20, 2018
This adds a project feature that allows for a project to specify that they would
like to skip submodule installation. Currently we are forcing all submodules to
be checked out, so this fails on private submodules.

Refs readthedocs/readthedocs-build#30
@stsewd
Copy link
Member

stsewd commented Mar 21, 2018

We'll add a new project.Feature, plus migration, to lock old users into the old behavior (all submodules are cloned)

I think this is kind of related to readthedocs/readthedocs.org#3821

What about versioning the spec of the yaml? I have raised an issue for that readthedocs/readthedocs.org#3806

So, in that case the users without a version key on the yaml, have a default version (1.0?). I know this would be a big change on the code base, but would allow us to make more changes in the future without worrying about the other projects.

For getting the submodules path we can parse the git submodule status command output or maybe parse the .gitmodules file so we can implement the exclude option.

include: all

I like the idea of the all keyword here, this keyword could be used on other parts of the spec. On formats for example.

agjohnson added a commit to readthedocs/readthedocs.org that referenced this issue Mar 22, 2018
This adds a project feature that allows for a project to specify that they would
like to skip submodule installation. Currently we are forcing all submodules to
be checked out, so this fails on private submodules.

Refs readthedocs/readthedocs-build#30
@stsewd stsewd mentioned this issue Mar 22, 2018
4 tasks
agjohnson added a commit to readthedocs/readthedocs.org that referenced this issue Mar 22, 2018
* Add temporary method for skipping submodule checkout

This adds a project feature that allows for a project to specify that they would
like to skip submodule installation. Currently we are forcing all submodules to
be checked out, so this fails on private submodules.

Refs readthedocs/readthedocs-build#30

* Also protect git clone

* Lint fixes
@agjohnson
Copy link
Contributor Author

Yeah, i think we need a version in our config, definitely.

@haubourg we have a stop gap measure in place for skipping all submodule checkout. What's your project? I can enable the feature if you don't want any submodules at all.

@haubourg
Copy link

@agjohnson Hi! our project is http://qwat.readthedocs.io/en/latest/.
If you can enable the submodule fence, that will help a lot thanks!

@humitos
Copy link
Member

humitos commented Apr 30, 2018

@haubourg I just added that flag in qwat project. Let me know if that worked for you. Thanks.

@haubourg
Copy link

haubourg commented May 2, 2018

@humitos that made the trick, many thanks !

@bochen87
Copy link

@agjohnson @humitos can you also please disable submodules for us? https://readthedocs.com/dashboard/enway-gmbh-cleansquare-ros

@humitos
Copy link
Member

humitos commented May 15, 2018

@bochen87 done! Let me know if it's OK :)

@bochen87
Copy link

@humitos works, thanks! :-)

@stsewd
Copy link
Member

stsewd commented May 15, 2018

Are we going to support this on the v1 of the yaml file? Or maybe to do this only on the v2.

@ericholscher
Copy link
Member

Sounds like we should probably support it in the v1. Seems like a useful option.

@sagark
Copy link

sagark commented May 21, 2018

@humitos Could you disable submodule cloning on https://readthedocs.org/projects/firesim/

We have similar issues with submodules that are currently private (and also very large), but unnecessary for building our documentation. For now, I've made a branch with .gitmodules ripped out to get our documentation up, but that's not ideal. Thanks a bunch!

@humitos
Copy link
Member

humitos commented May 22, 2018

@sagark done. It should skip the submodules now.

@sagark
Copy link

sagark commented May 22, 2018

Thanks, works for us now!

@agjohnson
Copy link
Contributor Author

Migrating this discussion to readthedocs/readthedocs.org#4464. We'll be implementing this feature there instead of this repo.

@dayeol
Copy link

dayeol commented Dec 4, 2018

@humitos Can you also disable submodules for https://github.com/keystone-enclave/keystone ?
I think the yaml feature is not being used in readthedocs.org yet is it?
Thanks!

Dayeol

@humitos
Copy link
Member

humitos commented Dec 4, 2018

@stsewd IIRC there is a setting to enable YAML v2 in some projects, right? If so, maybe this is a good opportunity to add keystone and let the user to use the skip submodules options from there. What do you think?

@adswa
Copy link

adswa commented Dec 4, 2018

I'm running into the same issue with a private submodule. However, I'm a complete newbie and just followed basic tutorial steps. My projects isn't very advanced and there could be hundreds of other things that have gone wrong, but it would be helpful to get the submodule problem out of the way to start fixing everything else that is potentially wrong - any advice would be appreciated. :-)
https://readthedocs.org/projects/multimatch/ from https://github.com/AdinaWagner/multimatch

@stsewd
Copy link
Member

stsewd commented Dec 4, 2018

@humitos yes we have submodules configuration working in v2, but the current schema will change soon, so I'm not sure if users want to keep an eye on the changes. And also the docs are not deployed yet.

@humitos
Copy link
Member

humitos commented Dec 4, 2018

@stsewd I was thinking about them helping us to test our V2 config as "beta tester users". I don't think they will need to use many of the configs though and just probably be enough with the simple ones + the exclude submodules. I don't know, just an idea.

@stsewd
Copy link
Member

stsewd commented Dec 4, 2018

That would be great to have test users. Basically, the config would be something similar to what we use on the rtd repo https://github.com/rtfd/readthedocs.org/blob/master/.readthedocs.yml

version: 2
sphinx:
  configuration: docs/conf.py
python:
  requirements: requirements.txt

Submodules are disabled by default in v2

@adswa
Copy link

adswa commented Dec 4, 2018

I gladly help as a beta tester, @stsewd & @humitos, and try out what you propose. I probably would need a bit a guidance, though. I have now included a .readthedocs.yml based on @stsewd 's suggestion at the root of my directory (https://github.com/AdinaWagner/multimatch/blob/master/.readthedocs.yml).
Currently, the build is failing with the error: "Problem in your project's configuration. Invalid "format": expected list".
The conf.py file is here (https://github.com/AdinaWagner/multimatch/blob/master/docs/source/conf.py) and was made with the quickstart wizard.
Could you give me some insights on how to fix this?

@stsewd
Copy link
Member

stsewd commented Dec 4, 2018

We need to activate a flag from our side to enable v2 for your project.

@adswa
Copy link

adswa commented Dec 4, 2018

sure - is there something I need to do at this point?

@humitos
Copy link
Member

humitos commented Dec 5, 2018

Hi @AdinaWagner and @dayeol!

I enabled YAML configuration V2 for your projects (mulitmatch and keystone). So, now you will need to create/edit the YAML file to something similar to what @stsewd mentioned at #30 (comment)

Note that you will need to remove/add the settings you need, but the key point is to use version: 2 at the top of the file.

Hope it works!

@humitos
Copy link
Member

humitos commented Dec 5, 2018

In case there is something wrong with this, open a new issue at https://github.com/rtfd/readthedocs.org/issues/new explaining this and linking this one there.

@adswa
Copy link

adswa commented Dec 5, 2018

Thanks, the docs built successfully! :-)

@humitos
Copy link
Member

humitos commented Dec 5, 2018

Awesome!

@stsewd
Copy link
Member

stsewd commented Dec 5, 2018

I'll let you know here when the schema change, here are some docs about the current implementation readthedocs/readthedocs.org#4451

@adswa
Copy link

adswa commented Dec 5, 2018

Thx, @stsewd !

@dayeol
Copy link

dayeol commented Dec 7, 2018

Hi, I added .readthedocs.yml and added version: 2 at the top, but still readthedocs is checking out all the submodules. Can you help?
I'm testing in test-rtfd-v2 branch:
yml: https://github.com/keystone-enclave/keystone/blob/test-rtfd-v2/.readthedocs.yml
conf.py: https://github.com/keystone-enclave/keystone/blob/test-rtfd-v2/docs/source/conf.py

@stsewd
Copy link
Member

stsewd commented Dec 7, 2018

@dayeol can you please provide your project url?

@dayeol
Copy link

dayeol commented Dec 7, 2018

@dayeol
Copy link

dayeol commented Dec 7, 2018

@stsewd Another problem is that SSL certificate is invalid, so the website shows red flag.
Thank you for your help!

@stsewd
Copy link
Member

stsewd commented Dec 8, 2018

@humitos believe you added the flag to the wrong project (keystone instead of keystone-enclave), the other project shouldn't have any problem anyway.

I think you have misconfigured the cname record, please see https://docs.readthedocs.io/en/latest/custom_domains.html

$ dig +short docs.keystone-enclave.org
keystone-enclave.readthedocs.io.
137.116.78.48

@humitos
Copy link
Member

humitos commented Dec 10, 2018

@stsewd Another problem is that SSL certificate is invalid, so the website shows red flag.

Please, open a new issue for this problem at https://github.com/rtfd/readthedocs.org/issues/new

@humitos
Copy link
Member

humitos commented Dec 10, 2018

@humitos believe you added the flag to the wrong project (keystone instead of keystone-enclave), the other project shouldn't have any problem anyway.

Added the flag to keystone-enclave now.

Hope it works now.

@NicolasMassart
Copy link

Hi @stsewd, as discussed on Gitter, could you enable v2 config on my site https://docs.pantheon.pegasys.tech/ ? Thanks a lot.

@humitos
Copy link
Member

humitos commented Dec 20, 2018

@NicolasMassart Done!

@NicolasMassart
Copy link

Thanks I will test that and tell you.

@stsewd
Copy link
Member

stsewd commented Jan 23, 2019

@NicolasMassart @AdinaWagner @dayeol

Hi, we just deployed some new changes to our v2 schema, so your projects will need to update the configuration file.

The main difference is in the requirements key.

What looks like this:

python:
   requirements: requirements.txt

You'll need to change it to

python:
    install:
        - requirements: requirements.txt

You can see the full updated docs in https://github.com/rtfd/readthedocs.org/blob/329286f2f8c811c6bda37f664546de73945dfe37/docs/config-file/v2.rst

or in the pull request readthedocs/readthedocs.org#4451

adswa added a commit to adswa/multimatch_gaze that referenced this issue Jan 23, 2019
@adswa
Copy link

adswa commented Jan 23, 2019

thanks a lot for letting me know @stsewd - I've implemented the change!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Accepted Accepted issue on our roadmap Feature New feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.