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: fixing repo related publish errors #46

Merged
merged 4 commits into from
Jan 15, 2020

Conversation

soldair
Copy link
Collaborator

@soldair soldair commented Jan 14, 2020

this fixes issues where no error message was printed when a repository
was missing or misconfigured. now you get something like

npm ERR! code E400
npm ERR! 400 Bad Request - PUT https://.... -
npm ERR!   ===============================
npm ERR!   Publish service error
npm ERR!   -------------------------------
npm ERR!   respository https://github.com/<repository> doesnt exist or <user> doesnt have access
npm ERR!   ===============================
npm ERR!

npm ERR! A complete log of this run can be found in:
npm ERR!     /..../.._34_372Z-debug.log

fixes #7

this fixes issues where no error message was printed when a repository
was missing or misconfigured. now you get something like

```
npm ERR! code E400
npm ERR! 400 Bad Request - PUT https://.... -
npm ERR!   ===============================
npm ERR!   Publish service error
npm ERR!   -------------------------------
npm ERR!   respository https://github.com/<repository> doesnt exist or <user> doesnt have access
npm ERR!   ===============================
npm ERR!

npm ERR! A complete log of this run can be found in:
npm ERR!     /..../.._34_372Z-debug.log
```
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 14, 2020
@@ -122,7 +122,7 @@ export const writePackage = async (
),
statusCode: 500,
};
res.end(ret.error);
res.end(JSON.stringify(ret));
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and below - can we use res.json instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm ok with using res.json.

I like to imagine that using methods only available on http servers makes things easier to move around but in practice no one ever does.

Copy link
Contributor

Choose a reason for hiding this comment

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

Heh, I come from the other side where I learned express far before the base level HTTP server, so I never know what's express and what's node, like the new jquery vs javascript

return ret;
}

let repoResp = null;
try {
repoResp = await github.getRepo(repo.name, user.token);
} catch (e) {
console.info('failed to get repo response for '+repo.name+' '+e)
Copy link
Contributor

Choose a reason for hiding this comment

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

Thoughts on using backticks and interpolation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm 🤷‍♂️
if you don't have new lines in your message back ticks are nearly as easy.

@soldair
Copy link
Collaborator Author

soldair commented Jan 14, 2020

i refactored error handling in write package to usea common function to write and format errors on the resonse. uses res.json, backticks and other suggestions. cc @JustinBeckwith

Copy link
Contributor

@JustinBeckwith JustinBeckwith left a comment

Choose a reason for hiding this comment

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

LGTM!

@bcoe bcoe merged commit d9d7049 into master Jan 15, 2020
@bcoe bcoe deleted the fix-repo-related-publish-errors branch January 15, 2020 00:36
@bcoe
Copy link
Contributor

bcoe commented Jan 15, 2020

awesome work @soldair, I like that you refactored the response logic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

better error message when repository field is missing
4 participants