-
Notifications
You must be signed in to change notification settings - Fork 4
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: set optional value for frameworkEnv, include schema/* in package #29
Conversation
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.
Looks good 🚀 👍
message: 'AC_JWKS_URL and AC_SIGNING_KEY_ALGORITHM is required for AutomationCloudJwt service', | ||
}) | ||
message: 'Check Environment: AC_JWKS_URL, AC_SIGNING_KEY_ALGORITHM', | ||
}); |
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.
Oh no, missing semicolon 🙀
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.
Oh no 😱 will put it back :)
src/main/services/auth.ts
Outdated
async authorize(_ctx: Koa.Context) {} | ||
getOrganisationId() { return null; } | ||
getOrganisationId() { return this.organisationId; } | ||
setOrganisationId(id: string) { this.organisationId = id } |
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.
Yeah, def something's off with semi
rule
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.
yup, it was off(probably one of the plug-in has it as off?) I've added it back 👍
}); | ||
} | ||
|
||
// subject to change the auth afterwards? |
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.
100%, my thoughts as well, we probably need to just pass Authorization
header to it.
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.
This is the case where I struggle with inversify, (and it happens fairly often with this Request!!)
should we add a method setAuthorization
and expect a user to always call it before using it to set header? or similar to that, make request
public so that they can just set the auth
? or just API_JOB_TIMELINE_AUTHORIZATION
as an env value? :D
API_JOB_TIMELINE_KEY
) doesn't have a default value, but when a user callsassertEnv
in their ownEnv
class,FrameworkEnv
's keys also validated, and it throws an error even though the app is not usingAPI_JOB_TIMELINE_KEY
. so I assigned empty string as a default value but added additional checking in each component's constructorschema/**/*
tofiles
property of package.jsonsetOrganisationId
for AuthServiceMock