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

feat(GHA): add req header containing file URL #735

Merged
merged 23 commits into from
Feb 1, 2023
Merged

Conversation

kanadgupta
Copy link
Member

@kanadgupta kanadgupta commented Jan 26, 2023

🚥 Fixes RM-6466

🧰 Changes

tl;dr we're adding two new request headers when hitting the ReadMe API! This will facilitate some upcoming changes (see RM-5907) 👀

Here are the two new request headers:

  1. x-rdme-ci: if the command is being run in a CI environment, this will be set to the name of the CI environment as determined by ci-info (e.g., GitHub Actions1)
  2. x-readme-source-url: if the command is syncing an OpenAPI or Markdown doc to ReadMe, this header2 contains the source URL for the file (if applicable). There are two ways that we determine the source file:
    1. If the command is being run via GitHub Actions and a local file is being synced, we use a variety of default environment variables to construct a full URL to the file that looks something like this.
    2. If the openapi command is being run and the file in question is located at an external URL3, that URL is used.

As expected, a bunch of other stuff was done in this PR to facilitate these changes:

  • I added a new restricted import ESLint rule for ci-info — it continues to be very annoying to write tests with and we should confine all usage of the package to our helper functions in src/lib/isCI.ts.
  • A buttload of tests (including a new test helper and rearranging a few existing tests) 🍑
  • Created a few types derived from oas-normalize (see fix: stricter type oas-normalize#248)
  • Enhanced our debug logs to include sanitized request headers for API requests
  • A few JSDoc additions/cleanups and variable renaming

🧬 QA & Testing

Well since each of these CI runs hit the ReadMe API to update this test project's reference docs, I was able to go into our Metrics dashboard for the ReadMe API and confirm that the request headers are showing up as expected:

CleanShot 2023-01-31 at 19 07 20@2x

If you go to the API Calls section in the dashboard, filter for my email address, and look for the rdme-test in the user column, you can click into any one of the API logs and peep the request headers. Note that the URL is off because we rely on unconventional checkout pathing logic in this workflow, but I feel good about these changes with the test coverage.

Footnotes

  1. See the full list here: https://github.com/watson/ci-info#supported-ci-tools (the Name column is what will be used)

  2. For commands like docs and openapi, we do a lot of GET requests as part of the general workflow, but this request header will only be added to the POST/PUT requests that directly update Markdown/OpenAPI files.

  3. For example, rdme openapi https://github.com/readmeio/oas-examples/blob/main/3.0/json/petstore.json

@kanadgupta kanadgupta added enhancement New feature or request command:docs Issues pertaining to the `docs`, `changelogs`, or `custompages` commands command:openapi Issues pertaining to the `openapi`, `validate`, `reduce`, or `swagger` commands API Underlying issue is with the ReadMe API and not necessarily the `rdme` client itself labels Feb 1, 2023
...payload,
}),
})
fetch(
Copy link
Member Author

Choose a reason for hiding this comment

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

I'd recommend hiding whitespace before reviewing the changes in this file!

src/lib/fetch.ts Outdated Show resolved Hide resolved
@kanadgupta kanadgupta marked this pull request as ready for review February 1, 2023 01:13
@kanadgupta kanadgupta requested a review from ilias-t February 1, 2023 01:16
__tests__/helpers/setup-gha-env.ts Outdated Show resolved Hide resolved
src/cmds/openapi/index.ts Show resolved Hide resolved
src/lib/fetch.ts Outdated Show resolved Hide resolved
src/lib/fetch.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@RyanGWU82 RyanGWU82 left a comment

Choose a reason for hiding this comment

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

Nothing really to note except for what Jon's already mentioned. The new headers sound potentially pretty useful, thanks for thinking of them.

__tests__/lib/fetch.test.ts Outdated Show resolved Hide resolved
__tests__/lib/fetch.test.ts Outdated Show resolved Hide resolved
@kanadgupta kanadgupta requested a review from erunion February 1, 2023 21:05
@kanadgupta kanadgupta merged commit 7574752 into main Feb 1, 2023
@kanadgupta kanadgupta deleted the file-url-header branch February 1, 2023 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Underlying issue is with the ReadMe API and not necessarily the `rdme` client itself command:docs Issues pertaining to the `docs`, `changelogs`, or `custompages` commands command:openapi Issues pertaining to the `openapi`, `validate`, `reduce`, or `swagger` commands enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants