-
Notifications
You must be signed in to change notification settings - Fork 133
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: when err.code is not numeric or doesnot exist take err.response.… #764
fix: when err.code is not numeric or doesnot exist take err.response.… #764
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
lib/file_transfer_agent/gcs_util.js
Outdated
@@ -178,7 +178,7 @@ function GCSUtil(httpclient, filestream) { | |||
encryptionMetadata | |||
); | |||
} catch (err) { | |||
const errCode = err['code'] ? err['code'] : err.response.status; | |||
const errCode = !err['code'] || isNaN(err['code']) ? err.response.status : err['code']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @naira-petrosyan-m. Thank you for your proposal. I tried to verify your code and in my opinion it could failed on for example boolean true. What do you think about:
!isNaN(err['code']) && !isNaN(parseInt(err['code'])) ? err['code'] : err.response.status ??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sfc-gh-pmotacki I just thought that err.code wouldn't be a Boolean value in any case, but sure I can change the condition to include parseInt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sfc-gh-pmotacki fyi I have pushed the update above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @naira-petrosyan-m. The code is correct now. I've run tests on your branch and got failed result. Could you rebase to master branch and verify code using 'npm run lint:check' please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sfc-gh-pmotacki my branch is opened from the "gcpoperationerror" branch, and when rebasing with master it gives a lot of conflicts as when rebasing master into "gcpoperationerror". This is what I get when running the lint checking command.
c8e2e81
to
1665e1a
Compare
I have read the CLA Document and I hereby sign the CLA |
* SNOW-1064052 - fix PUT files with wildcards on Windows
Hello @naira-petrosyan-m. After detect and fix bug in master branch it is needed to rebase branch with master. Can you rebase please? |
1665e1a
to
569af67
Compare
@sfc-gh-pmotacki I have rebased with master |
c79d086
into
snowflakedb:gcpoperationerror
Thank you one more time @naira-petrosyan-m |
…status instead
Description
GCP thrown err['code'] is a numeric value, while axios.head thrown err['code'] is a string name, e.g. BAD_REQUEST and it has a numeric err.response.status. Change includes checking if there is no err['code'] or it is not numeric take err.response.status otherwise take the err['code'].
Relates to issue SNOW-975540
and PR SNOW-975781
Checklist
npm run lint:check -- CHANGED_FILES
and fix problems in changed code)npm run test:unit
andnpm run test:integration
)