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 Notification for failure to add deploy key on manual imported project #11573

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

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Aug 29, 2024

@stsewd stsewd force-pushed the improve-deploy-key-failure-message branch from 8c19795 to 9554248 Compare September 3, 2024 17:53
@stsewd stsewd marked this pull request as ready for review September 3, 2024 23:52
@stsewd stsewd requested a review from a team as a code owner September 3, 2024 23:52
Comment on lines +108 to +113
We were unable to find this repository in your {{ provider_name }} account.
If this is a private repository, you'll need to
<a href="https://docs.readthedocs.io/page/guides/importing-private-repositories.html">
add the deploy key manually
</a>.
"""
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 we want to communicate this in a more direct way mentioning manual import or similar.

When adding a project manually, you need to add the deploy key manually. Follow this guide to add the deploy key manually.

ccing @agjohnson since I think he will have a better copy for this notification.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is closer to what I was thinking, however we should also cover public repositories with this notification. All manually created projects will still need to set up a webhook manually, and private repos will need a deploy key. The notification should probably more generically mention set up steps, the above is specific to deploy keys.

When adding a project manually, your repository will require extra configuration steps. Follow this guide to finish set up of your project.

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't we already mention the extra steps in the new dashboard?

Screenshot 2024-10-01 at 17-03-09 Add project - Read the Docs

That links to https://docs.readthedocs.io/en/stable/intro/add-project.html, which doesn't mention anything about SSH keys for private repos, we should mention that.

Anyway, I think it would be useful to show a notification after the project is imported, as a reminder to the user. But don't think we need to do that when adding an ssh key.

Copy link
Contributor

Choose a reason for hiding this comment

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

We mention that extra steps are required beginning project creation, but still want some notification afterwards that these are problems the user must address.

I would not mention the GitHub account, as this is not required to even be connected, and I wouldn't mention deploy keys specifically, as the project/repo is missing webhooks at this point too. The notification should be inclusive to both, as both.

Eventually, this could be onboarding steps with individual actions required, but we're not quite there yet.

Copy link
Member Author

@stsewd stsewd Oct 3, 2024

Choose a reason for hiding this comment

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

Yeah, I mean, we need a notification, but doesn't look like that notification is the one from this PR, since this notification was originally created on the ssh key setup step (I'm no longer using this notification in the .com PR).

"""
).strip(),
),
type=WARNING,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would be best as an info message, instead of warning message

Suggested change
type=WARNING,
type=INFO,

Comment on lines +108 to +113
We were unable to find this repository in your {{ provider_name }} account.
If this is a private repository, you'll need to
<a href="https://docs.readthedocs.io/page/guides/importing-private-repositories.html">
add the deploy key manually
</a>.
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

That is closer to what I was thinking, however we should also cover public repositories with this notification. All manually created projects will still need to set up a webhook manually, and private repos will need a deploy key. The notification should probably more generically mention set up steps, the above is specific to deploy keys.

When adding a project manually, your repository will require extra configuration steps. Follow this guide to finish set up of your project.

@ericholscher ericholscher removed their request for review October 2, 2024 13:36
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