-
Notifications
You must be signed in to change notification settings - Fork 4
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.
Code review done.
So it's the "OR" variant, just to extended the .env
template.
The option to promote this debugging flag should goes to OneSDK, right? The CLI execute
command does not care.
}): NewDotenv { | ||
const previousContent = previousDotenv ?? ''; | ||
|
||
const parameterEnvs = getParameterEnvs(providerName, parameters); | ||
const securityEnvs = getSecurityEnvs(providerName, security); | ||
const tokenEnv = makeTokenEnv(token); | ||
const logEnv = makeLogEnv(logEnabled === true ? '"on"' : '"off"'); |
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.
The motivation to change the default value is that user could have it already set up (e.g. using export
in the terminal)?
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 also don't see the point for this condition right now if the goal was to promote the env :)
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 just in case we might change the value. If you think that it is not needed I will remove it :)
}: { | ||
previousDotenv?: string; | ||
providerName: string; | ||
parameters?: IntegrationParameter[]; | ||
security?: SecurityScheme[]; | ||
token?: string | null; | ||
logEnabled?: boolean; |
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.
You are not passing this new argument. So it's only for potential use in the future?
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, left minor nitpick suggestions :)
}): NewDotenv { | ||
const previousContent = previousDotenv ?? ''; | ||
|
||
const parameterEnvs = getParameterEnvs(providerName, parameters); | ||
const securityEnvs = getSecurityEnvs(providerName, security); | ||
const tokenEnv = makeTokenEnv(token); | ||
const logEnv = makeLogEnv(logEnabled === true ? '"on"' : '"off"'); |
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 also don't see the point for this condition right now if the goal was to promote the env :)
Co-authored-by: Radek Kyselý <[email protected]>
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.
LGTM 👍
Description
Adds ONESDK_LOG= env variable to generated .env file with default value "off"
#355
Motivation and Context
Types of changes
Checklist: