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

Allow to disable shallow clone from the project's settings #5989

Closed
stsewd opened this issue Jul 24, 2019 · 12 comments · Fixed by #9016
Closed

Allow to disable shallow clone from the project's settings #5989

stsewd opened this issue Jul 24, 2019 · 12 comments · Fixed by #9016
Assignees
Labels
Feature New feature Needed: design decision A core team decision is required

Comments

@stsewd
Copy link
Member

stsewd commented Jul 24, 2019

We have a lot of this requests

#5988

I think we all agree that we should put this on the advanced settings tab.

The default option should be true. We can add a migration to migrate all projects that have the feature flag.

@stsewd stsewd added the Feature New feature label Jul 24, 2019
@humitos
Copy link
Member

humitos commented Jul 24, 2019

I think we added this because we had a problem of space in our builders and also most of the projects does not need it.

Also, I think the default should be to shallow clone and, when the user finds a problem, they can disable it.

Finally, if we are moving to isolated builds that will remove everything after each build, we can enable it by default --although, this will impact in the build time.

@humitos humitos added the Needed: design decision A core team decision is required label Jul 24, 2019
@stsewd
Copy link
Member Author

stsewd commented Sep 26, 2019

Yeah, I think even if we go for remove everything after build, this still is important to save us and the user build time.

@humitos
Copy link
Member

humitos commented May 25, 2021

I'd like to stop adding configs in our .readthedocs.yaml to just pass/remove an extra argument to a command that we execute automatically. Instead, I'd prefer to use something similar to the solution proposed in #8190 for all these cases: the user can just override build.jobs.checkout to write the git command as they want. In this particular case, to disable shallow clone.

@stsewd
Copy link
Member Author

stsewd commented May 25, 2021

I'd like to stop adding configs in our .readthedocs.yaml to just pass/remove an extra argument to a command that we execute automatically. Instead, I'd prefer to use something similar to the solution proposed in #8190 for all these cases: the user can just override build.jobs.checkout to write the git command as they want. In this particular case, to disable shallow clone.

Currently we parse the config file after clone, so we can't add that option there. We would need to change how we get the config file to support that.

@humitos
Copy link
Member

humitos commented May 26, 2021

Yeah, good point. I suppose we would need to find an alternative solution. An idea: "perform two checkouts: one to get the config file and another one to get the whole repository"

  1. check out the repository as it minimal version. Example: git --depth 1 --branch main --filter=blob:none
  2. read and parse the config file
  3. perform the checkout specified by the user

@astrojuanlu
Copy link
Contributor

Perhaps this is a bit of overkill for just loading the git configuration? Also, this would make the config version-wide, instead of project-wide. I agree with

  1. making shallow clones opt-out, rather than opt-in, and
  2. making this visible in the UI

@humitos
Copy link
Member

humitos commented Jun 2, 2021

@astrojuanlu it's not just about shallow clone setting, but about overriding the whole "potentially new" build.jobs.checkout step. So, we need to find a way to get that build.jobs.checkout step defined by the user before executing it. Enable/Disable shallow clone is just a good example of the usage/needing for overriding the checkout step.

On the other hand, we were trying to move away from settings only available on the UI and saved on the DB in the past year, and thinking that we should put new ones on the config file. The problem with this approach, as you already realized, is how we define project-wide configs --but that's a discussion that should happen in a different issue 😄

@dhellmann
Copy link

It may not be necessary to make such a large change to the builder to allow overriding the checkout command completely. If there was a configurable hook that ran after the normal shallow clone, it could be used to convert it to a full clone. The git-fetch docs mention an --unshallow option, for example, so a build.jobs.post_checkout parameter could be set to git fetch --unshallow for the simple case.

@marscher
Copy link

It may not be necessary to make such a large change to the builder to allow overriding the checkout command completely. If there was a configurable hook that ran after the normal shallow clone, it could be used to convert it to a full clone. The git-fetch docs mention an --unshallow option, for example, so a build.jobs.post_checkout parameter could be set to git fetch --unshallow for the simple case.

That sounds very reasonable to me and if I remember correctly was also the choice of several different CI providers in the past.

@humitos
Copy link
Member

humitos commented Mar 16, 2022

I tested this solution, #5989 (comment), with the PR that I'm working on at #9016 and it did work properly! 💯 So, I think that would be the suggested way to unshallow a git repository controlled by the user.

@ericholscher ericholscher moved this to Planned in 📍Roadmap Mar 21, 2022
@humitos humitos moved this from Planned to In progress in 📍Roadmap Mar 30, 2022
Repository owner moved this from In progress to Done in 📍Roadmap Apr 4, 2022
@humitos
Copy link
Member

humitos commented Apr 5, 2022

Hi! In a few hours, we are deploying a new feature that will allow people to call arbitrary commands at different moments in the build process (see #9016). For example,

build:
  os: ubuntu-20.04
  jobs:
    post_checkout:
      - git fetch --unshallow

I think this new config key, build.jobs, will cover this case. Take a look at the documentation at https://docs.readthedocs.io/en/latest/config-file/v2.html#build-jobs and please let me know if this is useful for you.

@marscher
Copy link

marscher commented Apr 5, 2022

Thank you so much for integrating this. RTD gains a lot of flexibility now!
edit: feature not yet deployed.

effigies added a commit to effigies/fmriprep that referenced this issue Jan 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature Needed: design decision A core team decision is required
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants