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/logging #47

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Fix/logging #47

wants to merge 3 commits into from

Conversation

busla
Copy link

@busla busla commented Feb 9, 2021

Without these envs in config.sh the wrapper fails in web mode:

export GITHUB_API_URL="https://api.github.com"
export GITHUB_LOGIN_URL="https://github.com"

I'll create a draft PR since I have a naive logger there that should be removed and you might want to have an input on how that is implemented for getUserEmails.

@TimothyJones
Copy link
Owner

Ah, nice catch. Apologies for that.

I reckon we could put the logging in gitHubGet and in check- which would let us know a bit more about what the user was up to.

There isn't currently a well-reasoned error handling system (because at the time I wrote this, the errors cognito received weren't exposed, so it didn't seem necessary). If you think it would be useful, we could implement custom error types that are then marshalled to appropriate http codes at the connector boundary.

Thinking about it, the specific error you hit could be caught and warned about - like if the logger said "Unable to retrieve users emails- does the app have appropriate permissions?" or something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants