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

Add temporary method for disabling shallow cloning (#5031) #5036

Merged
merged 1 commit into from
Dec 26, 2018
Merged

Add temporary method for disabling shallow cloning (#5031) #5036

merged 1 commit into from
Dec 26, 2018

Conversation

stephenfin
Copy link
Contributor

@stephenfin stephenfin commented Dec 24, 2018

This adds a project feature that allows us to use a standard clone and
fetch rather than the shallow clone/fetch introduced in #4939.
Eventually we should move this to the web UI, but doing so requires some
work to make sure, for example, that git options are only show when
'Project.repo_type' is 'git'.

Closes #5031

This adds a project feature that allows us to use a standard clone and
fetch rather than the shallow clone/fetch introduced in #4939.
Eventually we should move this to the web UI, but doing so requires some
work to make sure, for example, that git options are only show when
'Project.repo_type' is 'git'.

Signed-off-by: Stephen Finucane <[email protected]>
@humitos humitos requested a review from a team December 24, 2018 19:45
Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thanks.

def test_use_shallow_clone(self):
repo = self.project.vcs_repo()
repo.update()
repo.checkout('submodule')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be good to call .clone (mocked) here and check what were the arguments used to call it. That way, we are sure that we are using use_shallow_clone and it does have an effect in the flow.

Copy link
Member

@stsewd stsewd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we go for using a feature flag, this looks good for me


.. note::

Temporarily, we support skipping this option as builds that rely on
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we can remove this note if we go for using a feature flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thinking was to include this for now so we can unblock the broken builds then decide at a later date whether we wanted to expose this to users. If we don't, we remove the note. If we do, we remove all of this code.

Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I figured we'd need something like this, so glad to see it contributed, thanks!

@ericholscher ericholscher merged commit 408172e into readthedocs:master Dec 26, 2018
@ericholscher
Copy link
Member

Going to hotfix this in a deploy today to unblock you. 👍

We should also plan to document our user-impacting feature flags, so people know they can ask us to turn them on :)

@ericholscher
Copy link
Member

@stephenfin This should now be deployed. Let me know if it is working 👍

@ericholscher ericholscher added the PR: hotfix Pull request applied as hotfix to release label Dec 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: hotfix Pull request applied as hotfix to release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants