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 skipping submodule checkout #3821

Merged
merged 3 commits into from Mar 22, 2018

Conversation

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented 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

Based on #3822 for autolint cleanup

@agjohnson agjohnson requested a review from Mar 20, 2018
@agjohnson agjohnson added this to the Build stability milestone Mar 20, 2018
agjohnson added a commit that referenced this issue Mar 20, 2018
Pre PR autolinting
@agjohnson agjohnson force-pushed the agj/hotfix-add-submodule-skip branch from cd5a646 to d74f0a1 Mar 20, 2018
@agjohnson agjohnson changed the base branch from master to agj/cleanup/vcs-git Mar 20, 2018
@agjohnson agjohnson force-pushed the agj/hotfix-add-submodule-skip branch from d74f0a1 to 239b07c Mar 20, 2018
Copy link
Member

@ericholscher ericholscher left a comment

Looks good to me. Doesn't this require a migration?

This feels like something that we should be exposing to the users as an option, no? Having it be a "backend only feature" makes it so that only a support email can fix it, instead of just allowing users to turn it off. I'm not sure I really like this as a longer term pattern.

I realize this is just a quick fix to unbreak a client, but it still feels like something we should be adding to the YAML configs.

@agjohnson
Copy link
Contributor Author

@agjohnson agjohnson commented Mar 20, 2018

Yup, I think the delineation is features are short lived, there shouldn't be a UI for this. I agree the ideal solution is definitely YAML config, but there's no movement on readthedocs/readthedocs-build#30 just yet. No migration is needed for adding a Feature, though I think that was how I originally wrote the code. The admin form just does a lookup for field choices on instantiation.

@agjohnson agjohnson closed this Mar 20, 2018
@agjohnson agjohnson reopened this Mar 20, 2018
Copy link
Member

@humitos humitos left a comment

This looks good.

Echoing Eric, I'm fine with this as a temporary solution as the real solution is the YAML file.

I'm not sure if we ever remove old features after they were created, but it doesn't seem to interferes with the rest of the codebase and it doesn't seem to be a huge amount of code to remove anyway.

@humitos
Copy link
Member

@humitos humitos commented Mar 20, 2018

For some reason the latest commit didn't trigger tests. Can you check that?

agjohnson added a commit that referenced this issue Mar 20, 2018
* Autolint cleanup for #3821

Pre PR autolinting

* Fix unicode issue 🐍
@agjohnson
Copy link
Contributor Author

@agjohnson agjohnson commented Mar 20, 2018

Yeah, i'm not sure. I tried to manually trigger them and it didn't work. 😕

Edit: changed base to master, tests have been fired off

@agjohnson agjohnson changed the base branch from agj/cleanup/vcs-git to master Mar 20, 2018
return [code, out, err]

def clone(self):
code, _, _ = self.run(
'git', 'clone', '--recursive', self.repo_url, '.')
code, _, _ = self.run('git', 'clone', '--recursive', self.repo_url, '.')
Copy link
Member

@stsewd stsewd Mar 21, 2018

Should this also be separated? I mean deleting the --recursive option https://git-scm.com/docs/git-clone/2.8.0#git-clone---recursive and make the submodules init/update on other step.

Copy link
Member

@stsewd stsewd Mar 21, 2018

Also that option was dropped and now only --recurse-submodules is used on the latest git version https://git-scm.com/docs/git-clone#git-clone---recurse-submodulesltpathspec

Copy link
Member

@humitos humitos Mar 21, 2018

I think @stsewd is right here. We should remove the --recursive option. Otherwise, we will be trying to clone the submodules in the initial clone.

Copy link
Contributor Author

@agjohnson agjohnson Mar 22, 2018

Oh hrm, yeah. Good point!

agjohnson added 3 commits 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
@agjohnson agjohnson force-pushed the agj/hotfix-add-submodule-skip branch from 54edcf0 to 5a8a24b Mar 22, 2018
@agjohnson
Copy link
Contributor Author

@agjohnson agjohnson commented Mar 22, 2018

I just added the workaround for git clone step and rebased

@agjohnson agjohnson merged commit bd36494 into master Mar 22, 2018
1 check passed
@agjohnson agjohnson deleted the agj/hotfix-add-submodule-skip branch Mar 22, 2018
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.

None yet

4 participants