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

Connect Project with RemoteRepository Model to send Build Status Reports #6014

Closed
2 of 3 tasks
saadmk11 opened this issue Jul 30, 2019 · 9 comments
Closed
2 of 3 tasks
Labels
Needed: design decision A core team decision is required

Comments

@saadmk11
Copy link
Member

saadmk11 commented Jul 30, 2019

Details

To send build status reports to the providers ((GitHub, GitLab, Bitbucket)) we need to get a users token for that project. To get the token we are currently using the RemoteRepository Model so that we can get the social account of the user (i.e: project.remote_repository.account).

But There are many legacy projects in ReadtheDocs that do not have a remote repository connected or the users do not have a social account (GitHub, GitLab, Bitbucket) connected.
So, we need a good way to solve this issue to be able to send build status reports to the providers using their API.

Initial Proposal

There are mainly two way we could solve this issue for most of the users.

  1. We have an existing approach in our codebase which we may use

    for service_cls in registry:
    if service_cls.is_project_service(project):
    service = service_cls
    break
    else:
    log.warning('There are no registered services in the application.')
    project_notification.send()
    return None
    provider = allauth_registry.by_id(service.adapter.provider_id)
    notification = AttachWebhookNotification(
    context_object=provider,
    extra_context={'project': project},
    user=user,
    success=None,
    )
    user_accounts = service.for_user(user)
    for account in user_accounts:
    success, __ = account.setup_webhook(project)
    if success:
    notification.success = True
    notification.send()
    project.has_valid_webhook = True
    project.save()
    return True

    We can use something similar to this approach to solve the issue. we can pass the users of the project and check for their connected social accounts and try to send the status reports.
    For projects that don't have any users with a social account connected we can have a UI or site notification which will tell them to connect to a social account to get this feature available.

  2. This approach includes 2 steps.
    First: for new projects we need to have a way to create a remote repository while importing a projects manually and connect the project with that. But if the user importing the project does not have a social account that won't be much useful.
    Secondly: We need a UI for users to connect their project to remote repository maybe a button or something that will send a request to the providers API and fetch the repository data and create a remote repository. also in the UI we can tell the users why the build status might not be working for them.

There might be more approaches to this. these are what I came up with.
I would like to get @readthedocs/core teams thoughts on this and select an approach that will be best for us. :)

TODO:

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

humitos commented Jul 30, 2019

I do see different things here:

  • a. how to connect old imported projects to a RemoteProject? (Is is strictly required? --I think 1) does not need it)
  • b. how to get a valid Token to make the call?
  • c. how to communicate the User that their project is not connected?

I see 1) more robust in the way that the token used to make the call could change in time without issues (e.g. if the user who imported the project is not a maintainer anymore, then the next user's token will be used). Also, 1) is something that we are already doing. This way, if at least one of the maintainers has a Social account connected (with permissions on that GitHub repository) it will work.

What are the drawbacks for 1)? I'm not seeing them right now.

In all the cases, the communication to the user can be done using our Notification system.

@saadmk11
Copy link
Member Author

saadmk11 commented Jul 30, 2019

@humitos I agree with you. I think 1 is more cleaner approach and it will help us get around permission issues in most of the projects. and I also don't see any big issue with No. 1.

@ericholscher
Copy link
Member

I'm fine with #1, but what happens when a user tries to get PR builds working, they don't have a connected social account, and the PR builds just fail to work? We need to actively communicate the status of this to the user, and show them how to resolve the issue.

@ericholscher
Copy link
Member

I think we probably need some kind of setting for PR builds that users can turn on and off, and if it's turned on, then we show the status of the GitHub connection on the project detail page.

@saadmk11
Copy link
Member Author

I think we could have a Boolean Field on the project model like build_pr and if its turned on and user does not have a social account connected we can show a notification to the user about this and suggest them to connect a social account to get the feature working properly.

@ericholscher
Copy link
Member

@stsewd thoughts on whether this should be on the config file? It's a project-level setting, so I don't think it necessarily makes sense per-version on the config, so probably makes the most sense in the DB.

@stsewd
Copy link
Member

stsewd commented Jul 30, 2019

So, travis does this in the config file to allow users to skip the CI on some branches.

  • Do we want to offer that level of customization?
  • do we have cases where the user only wants to report the status on some branches?
  • Disabling the PR builder setting means don't report the status and not build the docs from the PR or just not report the status?

@saadmk11
Copy link
Member Author

saadmk11 commented Aug 1, 2019

Another approach for Communicating with the users: We can send site notifications about the issue when the send_build_status tasks fails and let the users know that they might not have a social account connected with the appropriate provider or they don't have permission.

@saadmk11
Copy link
Member Author

saadmk11 commented Aug 7, 2019

The main objective of this issue has been filled by #6017 and #6033. We can continue the discussion about Config file options for External versions from here #6046

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needed: design decision A core team decision is required
Projects
None yet
Development

No branches or pull requests

4 participants