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(@octokit/request-error) Fixes depreciation error when accessing error.headers #2064

Merged
merged 5 commits into from
Mar 4, 2022
Merged

fix(@octokit/request-error) Fixes depreciation error when accessing error.headers #2064

merged 5 commits into from
Mar 4, 2022

Conversation

daltonscharff
Copy link
Contributor

@daltonscharff daltonscharff commented Aug 27, 2021

What Changed

Updated @octokit/plugin-enterprise-compatibility to version 1.3.0 and modified the way error headers are accessed accordingly.

Why

A depreciation warning was being thrown by @octokit/request-error.

$ auto version
Deprecation: [@octokit/request-error] `error.headers` is deprecated, use `error.response.headers`.
    at RequestError.get (/Users/dalton.scharff/dev/auto-package-deployment-test/node_modules/@octokit/request-error/dist-node/index.js:64:24)
    at /Users/dalton.scharff/dev/auto-package-deployment-test/node_modules/@auto-it/core/dist/git.js:93:49
    at /Users/dalton.scharff/dev/auto-package-deployment-test/node_modules/before-after-hook/lib/add.js:37:18
    at runNextTicks (internal/process/task_queues.js:60:5)
    at listOnTimeout (internal/timers.js:526:9)
    at processTimers (internal/timers.js:500:7)
    at async Git.getCommit (/Users/dalton.scharff/dev/auto-package-deployment-test/node_modules/@auto-it/core/dist/git.js:204:26)
    at async Release.attachAuthor (/Users/dalton.scharff/dev/auto-package-deployment-test/node_modules/@auto-it/core/dist/release.js:382:34)

Issue 2049

Todo:

  • Add tests
  • Add docs

Change Type

Indicate the type of change your pull request is:

  • documentation
  • patch
  • minor
  • major

🐤 Download canary assets:

auto-linux--canary.2064.25372.gz
auto-macos--canary.2064.25372.gz
auto-win.exe--canary.2064.25372.gz

📦 Published PR as canary version: under canary scope @[email protected]

✨ Test out this PR locally via:

npm install @auto-canary/[email protected]
npm install @auto-canary/[email protected]
npm install @auto-canary/[email protected]
npm install @auto-canary/[email protected]
npm install @auto-canary/[email protected]
npm install @auto-canary/[email protected]
npm install @auto-canary/[email protected]
npm install @auto-canary/[email protected]
npm install @auto-canary/[email protected]
npm install @auto-canary/[email protected]
npm install @auto-canary/[email protected]
npm install @auto-canary/[email protected]
npm install @auto-canary/[email protected]
npm install @auto-canary/[email protected]
npm install @auto-canary/[email protected]
npm install @auto-canary/[email protected]
npm install @auto-canary/[email protected]
npm install @auto-canary/[email protected]
npm install @auto-canary/[email protected]
npm install @auto-canary/[email protected]
npm install @auto-canary/[email protected]
npm install @auto-canary/[email protected]
npm install @auto-canary/[email protected]
npm install @auto-canary/[email protected]
npm install @auto-canary/[email protected]
npm install @auto-canary/[email protected]
npm install @auto-canary/[email protected]
npm install @auto-canary/[email protected]
npm install @auto-canary/[email protected]
npm install @auto-canary/[email protected]
npm install @auto-canary/[email protected]
npm install @auto-canary/[email protected]
# or 
yarn add @auto-canary/[email protected]
yarn add @auto-canary/[email protected]
yarn add @auto-canary/[email protected]
yarn add @auto-canary/[email protected]
yarn add @auto-canary/[email protected]
yarn add @auto-canary/[email protected]
yarn add @auto-canary/[email protected]
yarn add @auto-canary/[email protected]
yarn add @auto-canary/[email protected]
yarn add @auto-canary/[email protected]
yarn add @auto-canary/[email protected]
yarn add @auto-canary/[email protected]
yarn add @auto-canary/[email protected]
yarn add @auto-canary/[email protected]
yarn add @auto-canary/[email protected]
yarn add @auto-canary/[email protected]
yarn add @auto-canary/[email protected]
yarn add @auto-canary/[email protected]
yarn add @auto-canary/[email protected]
yarn add @auto-canary/[email protected]
yarn add @auto-canary/[email protected]
yarn add @auto-canary/[email protected]
yarn add @auto-canary/[email protected]
yarn add @auto-canary/[email protected]
yarn add @auto-canary/[email protected]
yarn add @auto-canary/[email protected]
yarn add @auto-canary/[email protected]
yarn add @auto-canary/[email protected]
yarn add @auto-canary/[email protected]
yarn add @auto-canary/[email protected]
yarn add @auto-canary/[email protected]
yarn add @auto-canary/[email protected]

@adierkens adierkens added the minor Increment the minor version when merged label Aug 27, 2021
@reintroducing
Copy link

Any chance of getting the above PR merged in?

@sam-frampton
Copy link

+1 on getting this PR merged sometime soon

@hipstersmoothie
Copy link
Collaborator

Can you fix lint and update the PR?

@hipstersmoothie hipstersmoothie added patch Increment the patch version when merged and removed minor Increment the minor version when merged labels Jan 11, 2022
@adierkens adierkens added the minor Increment the minor version when merged label Jan 11, 2022
@codecov
Copy link

codecov bot commented Jan 11, 2022

Codecov Report

Merging #2064 (aad6559) into main (11021fa) will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2064      +/-   ##
==========================================
- Coverage   80.19%   80.17%   -0.02%     
==========================================
  Files          66       66              
  Lines        5413     5413              
  Branches     1263     1263              
==========================================
- Hits         4341     4340       -1     
- Misses        709      710       +1     
  Partials      363      363              
Impacted Files Coverage Δ
packages/core/src/git.ts 87.25% <0.00%> (-0.28%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 59b42d6...aad6559. Read the comment docs.

@matheusslg
Copy link

Any update?

@daltonscharff
Copy link
Contributor Author

I'm not sure how to get past this Codecov check. There's no longer any change to the git.ts file, but I'm still getting blocked.

@hipstersmoothie hipstersmoothie removed the minor Increment the minor version when merged label Mar 4, 2022
@hipstersmoothie hipstersmoothie merged commit 837aa6a into intuit:main Mar 4, 2022
@intuit-svc
Copy link

🚀 PR was released in v10.33.0 🚀

@intuit-svc intuit-svc added the released This issue/pull request has been released. label Mar 4, 2022
@daltonscharff daltonscharff deleted the daltonscharff/change-git-error-header branch April 5, 2022 00:33
@npvisual
Copy link

Note : I don't think that has been addressed as I am still seeing it in 10.36.5.

Is it possible to re-open the issue ?

The current warning is identical to the warning reported above.

% yarn --silent auto version
Deprecation: [@octokit/request-error] `error.headers` is deprecated, use `error.response.headers`.
    at RequestError.get (/Users/nicolasphilippe/Mperativ/GitHub/auto/node_modules/@octokit/core/node_modules/@octokit/request/node_modules/@octokit/request-error/dist-src/index.js:50:32)
    at /Users/nicolasphilippe/Mperativ/GitHub/auto/packages/core/src/git.ts:171:24
    at /Users/nicolasphilippe/Mperativ/GitHub/auto/node_modules/before-after-hook/lib/add.js:37:18
    at Git.getUserByUsername (/Users/nicolasphilippe/Mperativ/GitHub/auto/packages/core/src/git.ts:480:20)
minor
% yarn list --pattern auto
yarn list v1.22.18
warning package.json: No license field
warning No license field
├─ @auto-it/[email protected]
├─ @auto-it/[email protected]
├─ @auto-it/[email protected]
├─ @auto-it/[email protected]
├─ @auto-it/[email protected]
├─ @auto-it/[email protected]
├─ @auto-it/[email protected]
├─ @auto-it/[email protected]
├─ @auto-it/[email protected]
├─ @auto-it/[email protected]
├─ @auto-it/[email protected]
└─ [email protected]
✨  Done in 0.25s.

The problem seems to come from this statement in git.ts, since commenting that out will remove the warning :

    this.github.hook.error("request", (error) => {
      if (error) {
        // narrow down the type
        if ("headers" in error && error.request.headers.authorization) {
          delete error.request.headers.authorization;
          delete error.headers.authorization;                <-------- error.headers deprecated
        }
      }

The PR doesn't seem to modify this file at all.

I am not totally sure what the best course of action is here so please @hipstersmoothie advise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Increment the patch version when merged released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants