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

fix(deps): update octokit monorepo (major) #513

Merged
merged 22 commits into from
Jul 18, 2023

Conversation

renovate[bot]
Copy link
Contributor

@renovate renovate bot commented Jul 7, 2023

Mend Renovate

This PR contains the following updates:

Package Change Age Adoption Passing Confidence
@octokit/auth-action ^3.0.0 -> ^4.0.0 age adoption passing confidence
@octokit/core ^4.0.0 -> ^5.0.0 age adoption passing confidence
@octokit/plugin-paginate-rest ^7.0.0 -> ^8.0.0 age adoption passing confidence
@octokit/plugin-rest-endpoint-methods ^8.0.0 -> ^9.0.0 age adoption passing confidence
@octokit/types ^10.0.0 -> ^11.0.0 age adoption passing confidence

Release Notes

octokit/auth-action.js (@​octokit/auth-action)

v4.0.0

Compare Source

chore
BREAKING CHANGES
  • deps: bump version of @octokit/types to v11
octokit/core.js (@​octokit/core)

v5.0.0

Compare Source

Features
BREAKING CHANGES
  • Drop support for NodeJS v14, v16
  • Remove previews support for the REST API
  • remove agent option from octokit.request()
octokit/plugin-paginate-rest.js (@​octokit/plugin-paginate-rest)

v8.0.0

Compare Source

chore
BREAKING CHANGES
  • deps: Bump peerDependency of @octokit/core to v5
  • deps: Bump @octokit/types to v11
octokit/plugin-rest-endpoint-methods.js (@​octokit/plugin-rest-endpoint-methods)

v9.0.0

Compare Source

Bug Fixes
BREAKING CHANGES
  • deps: require @octokit/core > 5
  • deps: bump @octokit/types to v11
octokit/types.ts (@​octokit/types)

v11.1.0

Compare Source

Features

v11.0.0

Compare Source

Features
BREAKING CHANGES
  • Replace support for Node.js http(s) Agents with documentation on using fetch dispatchers instead

Configuration

📅 Schedule: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 Automerge: Disabled by config. Please merge this manually once you are satisfied.

Rebasing: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

👻 Immortal: This PR will be recreated if closed unmerged. Get config help if that's undesired.


  • If you want to rebase/retry this PR, check this box

This PR has been generated by Mend Renovate. View repository job log here.

@renovate renovate bot added the Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR label Jul 7, 2023
Copy link
Member

@wolfy1339 wolfy1339 left a comment

Choose a reason for hiding this comment

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

Requires updates to dependencies first

@renovate renovate bot changed the title fix(deps): update dependency @octokit/types to v11 fix(deps): update octokit monorepo (major) Jul 7, 2023
@renovate renovate bot force-pushed the renovate/major-octokit-monorepo branch 2 times, most recently from 59bf9a8 to 2042b52 Compare July 10, 2023 20:35
@nickfloyd
Copy link
Contributor

I'm working on the failing tests for this to unblock these dep updates.

@wolfy1339 wolfy1339 added the Type: Breaking change Used to note any change that requires a major version bump label Jul 11, 2023
Copy link
Contributor

@nickfloyd nickfloyd left a comment

Choose a reason for hiding this comment

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

Skipped tests need to be fixed

@nickfloyd
Copy link
Contributor

I ran into issues with how we mock fetch in the smoke tests, HttpProxyAgent and using something like undici. I need to read up on the APIs for that library to get a better idea of how to get this done properly. My code sandbox works fine but our fetch mocks do some heavy lifting so implementing this in tests always results in auth failure or something like it (see my scratch example below).

const { HttpProxyAgent } = require('https-proxy-agent');
const { Client } = require('undici');

async function main() {
  const proxyUrl = 'https://127.0.0.1';
  const targetUrl = '/repos/octocat/hello-world/issues';
  const agent = new HttpProxyAgent(proxyUrl);
  // Should be swapped with octokit.request w/ agent/dispatcher as an option
  const client = new Client(targetUrl, { agent });
  
  await client.request("POST /repos/{owner}/{repo}/issues", {
      owner: "octocat",
      repo: "hello-world",
      title: "My test issue",
  });
}

I am sure I am missing something obvious so If anyone else has a moment to take a look (@gr2m or @wolfy1339 or @oscard0m) I'd be glad for the additional 👀.

This is a blocker to getting https://github.com/octokit/request-action updated and out the door.

@renovate
Copy link
Contributor Author

renovate bot commented Jul 11, 2023

Edited/Blocked Notification

Renovate will not automatically rebase this PR, because it does not recognize the last commit author and assumes somebody else may have edited the PR.

You can manually request rebase by checking the rebase/retry box above.

Warning: custom changes will be lost.

@wolfy1339
Copy link
Member

The library used doesn't seem to be compatible with the Fetch API because it uses http.Agent from NodeJS

The fetch mock uses global.fetch which shouldn't be a problem as Node uses undici under the hood, the on;ly issue is the types aren't yet included in @types/node which is why we need to import undici's fetch method

The code will need to be updated, not just the tests

@wolfy1339
Copy link
Member

Also of interest, nodejs/undici#1650

package.json Outdated Show resolved Hide resolved
@kfcampbell
Copy link
Member

The test coverage failures here don't seem to me to be helping us in this instance. My instinct would be to suggest modifying the coverage threshold to unblock wrapping the Node/fetch breaking changes. I'm receptive to other suggestions for testing and challenges to my perception that the uncovered code is related to undici and not worth our time to test.

@wolfy1339
Copy link
Member

It seems that the HTTP proxy isn't getting used at all in the tests 😓 hence the coverage issues

@nickfloyd
Copy link
Contributor

It seems that the HTTP proxy isn't getting used at all in the tests 😓 hence the coverage issues

I'm digging in this morning to determine why... given the changes you proposed and the proxy tests all set the env, this should be working.

@wolfy1339
Copy link
Member

I did look into this some bit, and we override the custom fetch in the tests that is used in the production code. Since the custom fetch gets overridden the dispatcher option isn't set anymore

Copy link
Member

@kfcampbell kfcampbell left a comment

Choose a reason for hiding this comment

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

@oscard0m @wolfy1339 @gr2m @nickfloyd I've made some changes here to fix the failing tests and increase our test coverage. Please take another look and let me know what should be changed!

src/index.ts Show resolved Hide resolved
test/smoke.test.ts Show resolved Hide resolved
test/smoke.test.ts Show resolved Hide resolved
},
);
await customFetch("http://api.github.com", {});
expect(JSON.stringify(dispatcher)).toEqual(JSON.stringify(expectedAgent));
Copy link
Member

Choose a reason for hiding this comment

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

I believe these JSON.stringify hacks are similar to the others present we've been seeing and can be cleaned up when octokit/core.js#588 is addressed.

This reverts commit 6458785 in order to
increase code coverage.
Copy link
Contributor

@nickfloyd nickfloyd left a comment

Choose a reason for hiding this comment

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

💥

@nickfloyd nickfloyd requested review from kfcampbell and gr2m July 18, 2023 17:05
@kfcampbell
Copy link
Member

@wolfy1339 it looks like your review is required here due to the earlier request for changes. Do you mind taking another peek at this PR?

@kfcampbell
Copy link
Member

I have two final points before merging I'd like to address:

  • Are we okay merging with @gr2m's PR'd version of fetch-mock as a dependency? Is there a plan to get back to the main tree in the future?
  • This PR is currently set up to be a minor version bump, which is maybe a bit suspect as we're passing in different options to the Octokit client now. Should this be a major version bump?

@kfcampbell
Copy link
Member

kfcampbell commented Jul 18, 2023

I've talked with @nickfloyd a bit and we've decided to merge with the forked dependency and with a minor version bump as the interface hasn't had a breaking change.

@kfcampbell kfcampbell merged commit 3fd4ef6 into main Jul 18, 2023
@kfcampbell kfcampbell deleted the renovate/major-octokit-monorepo branch July 18, 2023 20:42
@github-actions
Copy link

🎉 This PR is included in version 6.0.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

@gr2m
Copy link
Contributor

gr2m commented Jul 18, 2023

  • Are we okay merging with @gr2m's PR'd version of fetch-mock as a dependency? Is there a plan to get back to the main tree in the future?

Absolutely, unfortunately the pull request is still open 🤷🏼

I think it would also be worth to look into undici's request mocking solutions and see if we can use that as a better replacement to fetch-mock. I feel fetch-mock is not actively maintained any more for a while now

@wolfy1339
Copy link
Member

Undici's does offer a mocking solution built-in, however it require the use of dispatchers, which means we can't test things like HTTP(s) proxies as they require dispatchers as well

https://undici.nodejs.org/#/docs/best-practices/mocking-request

@gr2m
Copy link
Contributor

gr2m commented Jul 18, 2023

we can't test things like HTTP(s) proxies

we already test proxies with a local server today without mocking requests. But yeah there could be other reasons that we cannot use it, but it's worth to explore it

@gr2m
Copy link
Contributor

gr2m commented Apr 19, 2024

fyi nodejs/undici#1650, new release is still outstanding at this time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Type: Breaking change Used to note any change that requires a major version bump Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants