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

Documentation names cleanup #8586

Merged
merged 13 commits into from
Oct 15, 2021
Merged

Documentation names cleanup #8586

merged 13 commits into from
Oct 15, 2021

Conversation

astrojuanlu
Copy link
Contributor

This fixes #8573, fixes a stray broken link, and addresses some confusion that arose at #8529. In summary:

  • Renames /webhooks to /integrations
  • Refers to "Integrations" rather than "Webhooks" when a generic term is appropriate or when we refer to our feature, leaves "webhooks" when speaking about a specific incoming webhook
  • Removes a confusing admonition on top of the "Edit Source integration" guide, that pointed the users towards a design document that was never implemented
  • Renames that "Edit Source" guide from /vcs to /edit-source
  • Improves the wording of that guide, since it was implying that the user had to add some variables to html_context. Read the Docs injects them anyway (just checked on a project using furo), so developers can focus on using them in their themes.
    • Like other things, it would be cool to have this as an extension instead.

We will need to create appropriate redirections for /webhooks and /vcs.

I wanted to do this before documenting #8522, since we want to have more in-depth documentation about (outgoing) webhooks.

@astrojuanlu astrojuanlu requested a review from a team October 14, 2021 10:54
docs/connected-accounts.rst Outdated Show resolved Hide resolved
docs/connected-accounts.rst Outdated Show resolved Hide resolved
docs/development/install.rst Outdated Show resolved Hide resolved
docs/guides/edit-source.rst Outdated Show resolved Hide resolved
docs/integrations.rst Outdated Show resolved Hide resolved
docs/pull-requests.rst Outdated Show resolved Hide resolved
@astrojuanlu
Copy link
Contributor Author

Thanks @stsewd for the feedback, addressed 👍🏽

@ericholscher
Copy link
Member

Refers to "Integrations" rather than "Webhooks" when a generic term is appropriate or when we refer to our feature, leaves "webhooks" when speaking about a specific incoming webhook

I'd like to see this in a style guide. Seems like we have a lot of implicit rules we should make explicit, so we can properly apply them across the team. 👍

Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

Looks good. Let's not forget to create the redirects for this.

docs/guides/developers.rst Outdated Show resolved Hide resolved
docs/integrations.rst Outdated Show resolved Hide resolved
docs/integrations.rst Show resolved Hide resolved
@astrojuanlu astrojuanlu enabled auto-merge October 15, 2021 14:15
@astrojuanlu astrojuanlu merged commit f22aaf8 into master Oct 15, 2021
@astrojuanlu astrojuanlu deleted the documentation-names-cleanup branch October 15, 2021 14:21
@ericholscher
Copy link
Member

ericholscher commented Oct 15, 2021

@astrojuanlu You didn't ship the redirect for this.

-> curl -IL https://docs.readthedocs.io/en/latest/webhooks.html
HTTP/2 404

@ericholscher
Copy link
Member

I went ahead and shipped the redirect now.

@astrojuanlu
Copy link
Contributor Author

Thanks, I was planning to do it before the deploy since we're hiding latest, but it's better to do it right after merging the PR actually

and with each change to your repository, Read the Docs is notified. When we
receive a webhook notification, we determine if the change is related to an
receive an integration notification, we determine if the change is related to an
Copy link
Member

Choose a reason for hiding this comment

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

In #8573 (comment) you said

We are consistent with other products

but GitHub calls these "Webhooks" (see https://github.com/readthedocs/readthedocs.org/settings/hooks), not "Integrations"

Anyways, I don't think we need to revert any of this work (at least now it makes sense under RTD itself), but I just want to mention that "Integration" is not a good name IMO and we should eventually stop using it :)

Copy link
Member

Choose a reason for hiding this comment

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

but GitHub calls these "Webhooks" (see https://github.com/readthedocs/readthedocs.org/settings/hooks), not "Integrations"

those are indeed webhooks, from the github side (you create a webhook to subscribe to github events). Then you have integrations https://github.com/readthedocs/readthedocs.org/settings/installations, that's services that subscribe to those events (that's what rtd does)

Copy link
Member

Choose a reason for hiding this comment

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

Well... Nope. If that's the case, we would be an "Integration" once we migrate to GitHub Application. --which is not the case, now.

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.

Clarify webhook and integration documentation
4 participants