readthedocs.org
readthedocs.org copied to clipboard
readthedocs
Bring the `preview` GH action into the product
We discussed about migrating the preview GH action into the product itself. The user should be able to mark Add link to preview in the pull request with comment and description as options.
I think we will need to use use the SocialAccount from one of the owners to put the link in the pull request as a comment or editing the description because we don't have a GitHub App for Read the Docs. We will need to research a little more here.
Note there are reason why most of these bots use a new comment, so we should probably use that approach only. Example, the content added to the description ends up in the commit message --which is something we don't want.
Sentry comment's example: https://github.com/readthedocs/readthedocs.org/pull/11765#issuecomment-2486053435
This would be good to start to figure out. Some things to explore:
- Is GHA required?
- Can we use a GHA in addition to our normal Oauth application, not changing authentication/authorization on our side yet?
There is some previous art regarding GHA in https://github.com/readthedocs/readthedocs.org/pull/8445 that I've done some time ago.
It would be good if we can prioritize this work since we have been jumping in/out into this work a few times already and we definitely need for a few different use cases, like less strict permissions.
@stsewd would you like to take on this initial research to answer the questions that Anthony raised? I know that django-allauth has changed in the last few months, so there may be something new we can take advantage of.
I think we should migrate to GH actions, I found a way that works without having two login options. The main idea is:
- A new user signs up using our new github app, nothing to do here.
- An existing user tries to sign in:
- The user is redirected to our new app so it can be approved (we should warn users that we are migrating to GH app and they will be required to grant access to this new app).
- We detect that this user already has an existing account with our old app.
- Instead of creating a new account, we just link the new social account to the existing user, and we log the user in.
That's the basic idea on how we can achieve the migration with just one login page. The other step is to get users to install the app into the repositories they have imported, that requires a little more of user interaction, but we can just link the users to our app with a list of repositories pre-selected so they can approve access, this would be something like this:
- The existing user is signed in, and his account has two social apps attached (the old and the new github integration).
- We know the repos that are imported under that account, we can create a single link for them to approve the instalation of the new app into those same repos (https://docs.github.com/en/apps/creating-github-apps/about-creating-github-apps/migrating-oauth-apps-to-github-apps#prompt-users-to-install-your-github-app).
- We need to change our remote repositories to have a reference to the installation, with that the RTD project will always be in sync with the repo (we have global webhooks now!)
- Once the user has migrated all of his repos to the new app (aka installed the app into the repos), we can disconnect him from the old GH oauth app, and also ask them to revoke access to that app from GH (as the GH guide suggests).
I already did a PoC with allauth, and it's possible to have two github apps running at the same time, and it's also possible to connect the new app to existing accounts on the fly (https://github.com/readthedocs/readthedocs.org/pull/11942/).
Once a project has been migrated to use the new gh app, it will be easier to integrate more features easily, like leaving a comment with a link to the PR preview, cloning private repos without having to worry about SSH keys, keep projects always in sync with repos, etc.
So, what do we want the comment to contain? Depending on that, we can decide from where to call the "post comment" function.
- Link to the root of the docs (what the GH action does)
- Show a summary of the FTD generated.
- Status of the build (this is probably redundant, as we already have this information in the commit status).
Also, do we want to have an option to edit the PR description? I think having only the option to post a comment is better, editing the PR description has many unwanted side effects, and may disrupt the user workflow. Alternative, we can also explore using the checks API https://github.com/readthedocs/readthedocs.org/issues/12022.
This is how a comment from our app looks like https://github.com/stsewd/rtd-test-builds/pull/1#issuecomment-2936224260
Link to the root of the docs (what the GH action does)
Yes.
Show a summary of the FTD generated.
Definitely! This is going to be a killer feature. Being able to click directly in the pages that changed it's gonna be great. I suppose we may want to put this behind a <details> HTML tab when there are more than 5 links. Otherwise, they can be just bullet points.
Status of the build (this is probably redundant, as we already have this information in the commit status).
This could be useful to know if the build finished already before clicking the links.
Also, do we want to have an option to edit the PR description?
I would avoid this due to the side effects, yes. We should only post a new comment in the PR as many other applications do. There even must be other reasons why they all behave in this way that we don't know about it yet.
Alternative, we can also explore using the checks API #12022
I'm not sure how that would work. I've never used the Checks tab. Do you have an example that shows the workflow?
Definitely! This is going to be a killer feature. Being able to click directly in the pages that changed it's gonna be great. I suppose we may want to put this behind a
<details>HTML tab when there are more than 5 links. Otherwise, they can be just bullet points.
Totally agree. Currently only the documentation root is published in the PR description. I often navigate to the pages that have changed in the PR preview, then copy and paste the URLs with their anchors into the PR description or subsequent comment to make it easier for reviewers to click and review for accuracy. If it ain't convenient, they'll skip the review.
@stevepiercy btw, we already have this feature as an addon, but sadly it isn't documented (https://github.com/readthedocs/readthedocs.org/pull/12185).
Definitely! This is going to be a killer feature. Being able to click directly in the pages that changed it's gonna be great.
My only worry is that it will be duplicating the feature from the addon, but I'll be fine with both.
This could be useful to know if the build finished already before clicking the links.
I prefer if we post the comment after the build and everything is ready, so the user gets notified about the comment when everything is ready, otherwise we will need to edit the comment, and no notification is created when a comment is updated.
I'm not sure how that would work. I've never used the Checks tab. Do you have an example that shows the workflow?
Codecov makes use of this feature https://github.com/readthedocs/readthedocs.org/pull/12223/checks
@stevepiercy btw, we already have this feature as an addon, but sadly it isn't documented (#12185).
Huh? That's visual diff.
I mean, I do something like this:
https://github.com/plone/volto/pull/7064#issuecomment-2870219221
Note that I had misconfigured the RTD PR preview, but fixed it in the next PR, which explains why there is nothing in the former PR's description.
Huh? That's visual diff.
You also get the list of files that have changed in the PR, you can see a live example here https://readthedocs-landing--12185.org.readthedocs.build/platform/12185/ (see the top right bar).
Ah, OK, that's a good workaround for injecting the list of links as a comment or into the description. I'll train the reviewers.
Or, better yet, I can add a note to use that menu in the PR template. Training complete!
I prefer if we post the comment after the build and everything is ready, so the user gets notified about the comment when everything is ready, otherwise we will need to edit the comment, and no notification is created when a comment is updated.
I'm viewing this in the opposite way. I'd like to post the comment immediately after the PR is opened:
- users will have all the information there ready (link to the build detail page, status of the build, etc)
- it will be always the first comment in the PR (there will be builds that take a few minutes, or even an hour where people can write comments on the PR during that time and leaving our comment down in the page at random places if we don't post it immediately)
- we can update the comment with new data once the build starts and finishes (status of the build, file tree diff links, etc)
- we don't bother users with a GH notification for each PR they open
users will have all the information there ready (link to the build detail page, status of the build, etc)
This information is already available in the commit status. I do feel like we are re-inventing that feature (in a non-standard way) if we include that information there.
we don't bother users with a GH notification for each PR they open
The notification will always be created, it doesn't matter when the comment is created.
This information is already available in the commit status. I do feel like we are re-inventing that feature (in a non-standard way) if we include that information there.
I don't think we are re-inventing anything here. We are adding all the useful links at hand, instead of have some of the data in the comment and some other data behind a few extra clicks. Putting everything in the comment makes this information to be available all in the same place.
The notification will always be created, it doesn't matter when the comment is created.
Yes, but it won't appear in your notification inbox most of the times because when the PR is opened, the user is already on the PR page and since the comment is immediately added, that comment will be mark automatically as read by GitHub. So, it won't generate an unread notification for that.
when the PR is opened, the user is already on the PR page
This is maybe assuming a bit much. Anecdotal, but I don't ever view PR pages when opening a PR. I use hub/gh cli tools to create issues and PRs. At very least, PR creation without visiting the UI is likely a common workflow though.
Which is to say we can expect to give users extra notifications in many cases anyways either way, so we might as well make sure the notification is worthwhile. I'd agree with @stsewd that updating with the final build information would consistently be the most useful.
I don't think we are re-inventing anything here. We are adding all the useful links at hand
I'd also agree with @stsewd here too. The commit status already shows the build status in a more accurate way, so that specifically seems like a redundant feature. A duplicate link to the build detail page doesn't seem super useful, but it's static information that doesn't require mutable comments at least.
The most important part of this feature is to link more accurately into the built documentation and pages that changed. I think at very least for a first pass we should keep the implementation simple. This feels like immutable comments, updated once after a build finishes successfully.
Another option would be to implement the checks API, which basically combines both, the status of the build and additional information. Checks are displayed with normal commit statuses, and link to the checks page (codecov does this https://github.com/readthedocs/readthedocs.org/pull/12242/checks). Of course, the downside would be that the information won't be as visible as a comment.
Ah interesting, maybe using the checks UI is something to consider later too. I would still agree that comments are likely the place where the links are most likely to get clicked though. That seems to still be really common pattern with GitHub integrations.
This is maybe assuming a bit much. Anecdotal, but I don't ever view PR pages when opening a PR. I use
hub/ghcli tools to create issues and PRs. At very least, PR creation without visiting the UI is likely a common workflow though.
I'm not assuming too much here. Opening a PR from the WebUI is the common workflow on GitHub. We are pretty advanced users with different setups that may not follow this common workflow, but most of our users will.
The commit status already shows the build status in a more accurate way, so that specifically seems like a redundant feature.
Why we would hide this useful link under 1) scrolling down to the bottom of the page, 2) click on "All checks have passed" and 3) click on "docs/readthedocs.com:read-the-docs-ext-theme — Read the Docs build succeeded!" (which depending on the state of the build, it points to a different place) -- when we can just click on a "Build details page" link at 1 click and pretty explicit name?
Also note that you cannot access the "Build details page" once the build has finished, since that link will point to the documentation built from the PR instead.
That said, I think have explicit links in the comment is a lot better/explicit and less confusing UX.
This feels like immutable comments, updated once after a build finishes successfully.
The comment won't be immutable since we still need to update it on every push to the PR to update the build status (success, failed, etc) and also to update the file tree diff files. We will always need to update the comment, even if we post the first one once the build is finished.
Also note that you cannot access the "Build details page" once the build has finished, since that link will point to the documentation built from the PR instead.
I'm fine linking to the build page, what I'd avoid is putting the actual status of the build in the comment.
Opening a PR from the WebUI is the common workflow on GitHub
Users can use vscode, web UI, CLI, or might get notifications through code owners even.
My point was there is no way to guarantee users won't get a notification for an unhelpful placeholder comment so we shouldn't plan our design around this.
Why we would hide this useful link
Build status in comments is redundant to what GitHub already does for us with check status, so it's an easy place to avoid complexity.
The build detail link isn't a problem to include. But it's at worst not super useful to users. On builds passing the build detail page isn't very helpful, on builds failed users already know to use the check status UI.
The comment won't be immutable since we still need to update it on every push to the PR to update the build status
Without build status, it doesn't seem like updating comments has a super strong use case.
With comments, commits, and references showing sequentially on a PR, it would be clear UX if our comments read sequentially too.
Sequentially adding comments, it would be extra helpful if our comments in a PR were as granular as:
- "Documentation build 123 passed, files changes: /en/latest/bar, /en/latest/foo"
- Comment
- Comment
- Commit adding /en/latest/blah
- "Documentation build 124 passed, new files changed: /en/latest/blah"
Overall, starting from appending comments on build success is an easy first iteration that we can use to get user feedback. I'd follow what other service integrations do here, which it seems appending comments is the most common pattern.
having one comment per commit may overwhelm users and will probably make them disable the feature... even if the comment is very minimal, it adds noise to the PR page.
We should have one and only one comment. Then, on every push, we should update that comment with the latest updates. That's what I'm saying.
Yeah it could absolutely be too noisy, but it seems like an okay place to start and see what users think.
What I was describing was only commenting if a build causes updates to the list of files. Otherwise, the comments wouldn't have build state and the links wouldn't have changed, so a new comment wouldn't be necessary. PRs would only get spammed if the user adds many commits each changing new files.
Most PRs should still only have a single comment, but we aren't going to work to update it constantly like we do build state.
So, I don't feel we need to start with a single comment
What I was describing was only commenting if a build causes updates to the list of files. Otherwise, the comments wouldn't have build state and the links wouldn't have changed, so a new comment wouldn't be necessary. PRs would only get spammed if the user adds many commits each changing new files.
This adds a lot of unneeded complexity. We need to build this logic and decide if we have to comment or not. I'm sure it will be hard to make it reliably.
It's a lot easier to always update the comment with the latest update without caring what was the previous state.
even if the comment is very minimal, it adds noise to the PR page
I agree with @stsewd here
The build detail link isn't a problem to include. But it's at worst not super useful to users. On builds passing the build detail page isn't very helpful, on builds failed users already know to use the check status UI.
We built a full GitHub action to put the exact same link we put in the status checks into the description. Users loved it! Why we don't want to add it in the comment now? 🤔
We just need to replicate that feature here and put that same link at the hand of people instead of hiding it as I described before.
Most PRs should still only have a single comment, but we aren't going to work to update it constantly like we do build state.
Not sure if this is true, docs PRs can spam changes to other files during review, or just the user opening the PR in a WIP state, and just adding changes. We will be creating one comment per commit, but even then if we optimize the algorithm to just create a couple of comments, users will need to scroll down to search for the comment with the most up-to-date information, all the other comments will just add noise at the end.
Yeah, there are definitely edge cases that could be spammy, but we're also a bit early to worry about edge cases. Continually updating a single comment does has poor UX in edge cases as well, especially for long threads or long lived threads. Having a comment that is the only non-sequential item in a list of sequential comments/commits/etc will be awkward UX.
I feel using multiple comments could be a better experience as the feature matures, but I also feel we should start wherever is easiest. Neither option are good UX -- it's just the best we can do to hack our feature into GitHub. If the checks UI is actually discoverable, that is closer to the UI we need and I would ditch using comments -- it just feels hidden though.
My main concerns above were that we aren't rebuilding GitHub features like check status in our comments, and we aren't modeling our comments as a duplication of our build status, like showing failed builds/etc.
In my experience multiple automated comments add a lot of noise to the workflow and make it hard to follow the conversation in the PR.
Also, I'd prefer to start simple without any "magic algorithm" behind it and always behave in the same way without making any decision dynamically. My proposal is as follow:
- the PR is opened
- we detect that and we post a comment immediately (it will likely be always the first comment in the PR) saying the build is triggered but hasn't finished yet with the build number, a link to the build details page and other stuffs we want.
- when the build finished, we update the comment with the new data we have (success, failed status and file tree diff, link to the built docs)
- when a new push arrives, we do 2) and 3) again, but instead of posting a new comment in 2) we just update it to say a new builds was triggered
This looks like a pretty simple approach in my mind and easy to understand and follow. It always behaves in the same way no matter what. No surprises.
To clarify I don't care where we start re single comment or multiple comments, but I am saying multiple comments will end up being better UX in more cases.
But I'm still -1 on adding a placeholder comment stating the build is in progress. Users will get a notification about our placeholder comment, the comment content is rather unhelpful, and that comment will be in a placeholder state until the build finishes -- so it could be there for 10-60+ minutes.
I haven't seen another GitHub integration do this, so I don't think we should stand out with this UX. I still think it's better to start with posting an update only after the build finishes.