-
Notifications
You must be signed in to change notification settings - Fork 87
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
feat: deprecate MYINFO_APP_KEY #557
Conversation
} else if (config.isDev && process.env.MYINFO_APP_KEY) { | ||
myInfoConfig.appId = 'STG2-' + myInfoConfig.singpassEserviceId | ||
myInfoConfig.privateKey = process.env.MYINFO_APP_KEY | ||
myInfoConfig.mode = 'stg' |
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.
do we no longer need a stg
mode?
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.
we do, but it's set via the env var MYINFO_CLIENT_CONFIG
, which is set to stg
in our staging environment. MYINFO_APP_KEY
is unused.
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.
will this break the development integration with mockpass?
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.
so if I'm not wrong, the purpose of this is to have a dev environment that talks to MyInfo dev instead of MyInfo mockpass? MyInfo has 4 different environments if I'm not wrong; Prod, staging, dev and mockpass. Am i correct @liangyuanruo ?
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.
I've tested myinfo on mockpass and it works fine. we don't use this env var in our dev environment, we only use MYINFO_CLIENT_CONFIG
.
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.
isn't the purpose of this just so that you can specify a key directly in the env vars instead of specifying a file path? why would it be listed as "deprecated" in our documentation if it's required for something?
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.
@arshadali172 do you mean the myinfo-gov-client mode? that mode is specified using MYINFO_CLIENT_CONFIG
Problem
The environment variable
MYINFO_APP_KEY
is still present in the code for our non-production environments, even though it is no longer used. It should be fully deprecated so that we can migrate the SPCP/MyInfo code to TypeScript without worrying about supporting it. Moreover, open-source users should be able to onboard MyInfo without being confused betweenMYINFO_APP_KEY
andMYINFO_FORMSG_KEY_PATH
.Solution
Deprecate
MYINFO_APP_KEY
and remove it from the documentation.