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

Manual project import: Fails to detect Github "main" branch #9367

Closed
benjaoming opened this issue Jun 22, 2022 · 11 comments · Fixed by #9424
Closed

Manual project import: Fails to detect Github "main" branch #9367

benjaoming opened this issue Jun 22, 2022 · 11 comments · Fixed by #9424
Assignees
Labels
Accepted Accepted issue on our roadmap Bug A bug

Comments

@benjaoming
Copy link
Contributor

Details

Leaving the "Default branch" field empty does not work, but will start to build master branches.

Expected Result

  • Importing a Github project, should immediately start building the default branch -- and if no default branch can be detected, then main should be the default
  • Help text should mention main not master

Actual Result

git clone --no-single-branch --depth 50 https://github.com/readthedocs-examples/example-sphinx-basic .
git checkout --force master
error: pathspec 'master' did not match any file(s) known to git

image

@benjaoming benjaoming added the Bug A bug label Jun 22, 2022
@benjaoming benjaoming changed the title Manul project imports fail to detect Github "main" branch Manual project import: Fails to detect Github "main" branch Jun 22, 2022
@stsewd
Copy link
Member

stsewd commented Jun 22, 2022

Importing a Github project, should immediately start building the default branch -- and if no default branch can be detected, then main should be the default

Currently the default is master, and if we change the default to main the same error will be faced when someone imports a project that doesn't have a branch named main.

We could reword the help text and say the default is master, leaving out the "default value of your VCS".

@benjaoming
Copy link
Contributor Author

Is it not possible to detect which branch is default?

@stsewd
Copy link
Member

stsewd commented Jun 22, 2022

Is it not possible to detect which branch is default?

Currently only if the project wasn't manually imported.

@benjaoming
Copy link
Contributor Author

In addition to changing the help-text so it isn't directly misleading, another possible work-around would be to hard-code the initial value of the "Default branch" to main for manual imports? So it will always appear like this, forcing the user to take action?

image

@stsewd
Copy link
Member

stsewd commented Jun 30, 2022

We could also try setting the default branch if the checkout step fails and the default version isn't set, something around these lines

if not identifier:
identifier = self.default_branch or self.fallback_branch
identifier = self.find_ref(identifier)

@humitos
Copy link
Member

humitos commented Jul 2, 2022

I think we shouldn't guess the default branch or try anything customized on Read the Docs. Why not just default to the branch that git defaults by default? I mean, we should remove the step git checkout --force master when we don't have a default branch manually defined. Removing that step, everything should work. Git will use the branch cloned by default when doing git clone. That's it.

@humitos
Copy link
Member

humitos commented Jul 11, 2022

I got another customer hitting this problem. This bug blocks them when importing projects manually (e.g. because they are using Azure DevOps) and generates a bad UX at onboarding.

humitos added a commit that referenced this issue Jul 11, 2022
Currently, importing a project manually without setting the `default_branch`,
will make Git VCS to use `master` as the default branch. However, with the last
swap about `master` and `main` branches, repositories defaulting to `main`
branch fail when being imported on Read the Docs. Leaving the user in a blocked
state they can get out.

This commit allows `Version.identifier` to be NULL meaning that "it won't try to
guess what's the default branch of the VCS" and just leave the repository in the
immediate state after being cloned (which is already the default branch). To do
this, I'm removing the command `git checkout --force <branch>` from the steps
executed for this case.

Closes #9367
@agjohnson
Copy link
Contributor

Why not just default to the branch that git defaults by default?

This seems like a good approach. Do we need that text input value to establish the latest Version object though?

@benjaoming
Copy link
Contributor Author

benjaoming commented Aug 9, 2022

I tried the method in this StackOverflow solution and I like it.

git remote show [your_remote] | sed -n '/HEAD branch/s/.*: //p'

I would propose to parse the git remote show output in Python instead of sed.

Tested with:

  • github.com ✔️
  • gitlab.com ✔️
  • my own private SSH bare git repo ✔️
  • bitbutcket.org ✔️

This seems to work with:

  • Any currently checked out local branch
  • Doesn't matter if remote is ssh or https
  • Running the command needs to connect to the upstream repo - if ssh credentials do not work, the command fails

NOTICE

git help remote
# ...
       show
           Gives some information about the remote <name>.

           With -n option, the remote heads are not queried first with git ls-remote <name>; cached information is used instead.

We can add the option -n to cache. This makes the command extremely fast, and since it's a freshly cloned repo, perhaps that's what we want?

The reason I did these tests was a remark on StackOverflow saying The default branch is a github thing, not a git thing but I don't think anymore that the above method has anything specific to do with GitHub.

@humitos humitos added the Accepted Accepted issue on our roadmap label Aug 30, 2022
humitos added a commit that referenced this issue Jan 25, 2023
…9424)

* Git backend: make `defaul_branch` to point to VCS' default branch

Currently, importing a project manually without setting the `default_branch`,
will make Git VCS to use `master` as the default branch. However, with the last
swap about `master` and `main` branches, repositories defaulting to `main`
branch fail when being imported on Read the Docs. Leaving the user in a blocked
state they can get out.

This commit allows `Version.identifier` to be NULL meaning that "it won't try to
guess what's the default branch of the VCS" and just leave the repository in the
immediate state after being cloned (which is already the default branch). To do
this, I'm removing the command `git checkout --force <branch>` from the steps
executed for this case.

Closes #9367

* Git backend: use `default_branch` to decide whether build latest

Use `default_branch` coming from the Webhook itself to decide if a build for
Latest Version with `identifier=None` has to be triggered or not.

Note that this feature only works for GitHub and GitLab since Bitbucket does not
send the `default_branch` of the repository.

* Update `Version.identifier` based on VCS's `default_branch`

The VCS's `default_branch` comes in the incoming webhooks that GitHub and GitLab
sends to us when the user realizes an action in the repository.

Note that in this commit BitBucket is not supported and will keep having the
issues this PR solves. To solve this problem, we would need to update the
`Version.identifier` from the builder immediately after clonning the default
branch (e.g. `git clone <URL>`). I put some commented code/ideas in place to
come back to this once we feel it's the right time.

* Migrations dependencies

* Lint

* Add note about how to expand use cases for this command

* Revert "Add note about how to expand use cases for this command"

Meh, I made a mistake. This commit should have gone to a different branch.

This reverts commit fc90d36.

* Apply suggestions from code review

Co-authored-by: Eric Holscher <25510+ericholscher@users.noreply.github.com>

* Typo

* TODO comment to potentially remove `identifier` from STABLE

* Tests: adjust our tests to match the new changes

* Lint

* Webhooks: add support for `default_branch` in generic webhooks

People is able to pass `default_branch=main` so Read the Docs can decide whether
or not it has to trigger a build for `LATEST` in case its `identifier=None` (the
default branch of the repository)

* Tests: define `default_branch="master"`

All these tests are expecting `master` to be the default branch.
We have to force this now since it was automatically "guess" previously,
but now we are not guessing anymore.

* Tests: remove `allow_deprecated_webhooks` from this check

I don't understand why we were checking for this in this test,
but I think it's not necessary since it's an old feature.

* Docs: mention it's optional

* Test: this feature flag is required

I removed because executing tests with `--nomigrations` made it fail.

Co-authored-by: Eric Holscher <25510+ericholscher@users.noreply.github.com>
@FrostyX
Copy link

FrostyX commented Jan 25, 2023

The "default branch" field is not a text field anymore, it is a select now. Which is a bit problem, when the main branch is missing. I ended up right-clicking to inspect and changed the

<option value="master" selected="">master</option>

to

<option value="main" selected="">main</option>

In case anyone needs to workaround the issue as well.

@benjaoming
Copy link
Contributor Author

That workaround seems good @FrostyX - but a fix has also been committed in #9424 and is scheduled to be rolled out at the next release, probably early next week 🎉 If you happen to encounter any further issues, please let us know 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Accepted issue on our roadmap Bug A bug
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants