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

Project: use clone URL from connected repository if available #11826

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Dec 4, 2024

  • If the user selects a remote repository, the repo field is hidden, if the user doesn't select a remote repo, they can edit the field.
  • We always try to use the clone url from the remote repo if available
  • When importing a project, the repo field is shown as read-only (except on manual import of course).

We have close to 7K projects with a remote repo attached with a different clone URL on .org, and 700 on .com.

In [4]: Project.objects.filter(remote_repository__isnull=False).filter(~Q(repo=F('remote_repository__clone_url'))).count()
Out[4]: 6869

In [1]: Project.objects.filter(remote_repository__isnull=False).filter(~Q(repo=F('remote_repository__clone_url'))).count()
Out[1]: 739

Do we care? Should I analyze those further? Should we contact users?

Closes https://github.com/readthedocs/readthedocs-corporate/issues/1859

@stsewd stsewd marked this pull request as ready for review December 5, 2024 23:22
@stsewd stsewd requested a review from a team as a code owner December 5, 2024 23:22
@stsewd stsewd requested a review from humitos December 5, 2024 23:22
@humitos
Copy link
Member

humitos commented Dec 9, 2024

Do we care? Should I analyze those further? Should we contact users?

How many of them are active projects (not spam, built in the last 180 days, active subscription, etc)?

Comment on lines 72 to 74
# The clone URL will be set from the remote repository.
if self.instance.remote_repository:
self.fields.pop("repo")
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 that @agjohnson mentioned he wanted to keep showing the URL but making it read-only.

Copy link
Contributor

Choose a reason for hiding this comment

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

A read only field might be the easiest for now, yeah. We might also want to replace the remote repo field with a better widget eventually too, which would then allow us to remove the read only field later. As long as we give the maintainers are clear idea of the URL we clone from that should be fine for now though.

Copy link
Member Author

Choose a reason for hiding this comment

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

The remote repository field already shows the clone URL that's being used, are we okay duplicating that information?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, that seems fine. We probably eventually just want this to be one field: when using a manual repo URL the text field shows, when using a remote repo the dropdown shows. There needs to be UI that allows projects to switch though.

So for now, showing both fields always seems the least confusing.

readthedocs/projects/models.py Show resolved Hide resolved
@stsewd
Copy link
Member Author

stsewd commented Dec 9, 2024

How many of them are active projects (not spam, built in the last 180 days, active subscription, etc)?

That leaves 853 projects on .org and 183 on .com

from datetime import timedelta

from django.db.models import F, Q, Sum
from django.utils import timezone

from readthedocs.projects.models import Project

projects = (
    Project.objects.filter(remote_repository__isnull=False)
    .filter(~Q(repo=F("remote_repository__clone_url")))
    .annotate(spam_score=Sum("spam_rules__value", default=0))
    .filter(spam_score__lt=300)
    .filter(organizations__stripe_subscription__status='active')
)

different = []
six_months_ago = timezone.now() - timedelta(days=180)
for project in projects.select_related("remote_repository"):
    clone_url = project.remote_repository.clone_url
    ssh_url = project.remote_repository.ssh_url
    if f"{project.repo}.git" != clone_url and project.repo != ssh_url:
        if project.builds.filter(
            state="finished", success=True, date__gte=six_months_ago
        ).exists():
            different.append(project)

@humitos
Copy link
Member

humitos commented Dec 12, 2024

183 on .com

This looks a little bit high to me. I wasn't expecting so many. We may need to take a deeper look to understand why these are the cases and if the URLs is completely different and not something obvious (repository moved or similar), we may want to contact those customers. I don't think we will find something like that, tho.

@stsewd
Copy link
Member Author

stsewd commented Dec 12, 2024

I'm +1 on emailing users, changing their clone URL without any warning will likely break their docs. I see that there are some projects that have a matching name with only the owner being changed, but that can also mean that someone set up a separate repo to test things out and then moved it to the real repo (I've seen cases like this in support requests).

But I'm also not sure how confusing it can get for users to receive an email like this... Like, we will be asking them to change the connected repo to another one that matches the clone URL. Maybe we can try to that change automatically for some users? (Find a remote repo they have access that matches the current clone URL). But I'm also sure we will get into trouble with users not having access to that repo and no being able to link it.

@humitos
Copy link
Member

humitos commented Dec 16, 2024

I understand the mismatching URL is important only for organizations that have GitHub SSO enabled (since it permissions are based on the RemoteRepository connected). We should focus only on those projects to contact users. It may be a very small chunk from those 183.

@stsewd
Copy link
Member Author

stsewd commented Dec 24, 2024

I understand the mismatching URL is important only for organizations that have GitHub SSO enabled (since it permissions are based on the RemoteRepository connected). We should focus only on those projects to contact users. It may be a very small chunk from those 183.

At the contrary, permissions won't change for those, since we already use the current connected repo to check for authz.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants