-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Cleanup env variables #24739
Cleanup env variables #24739
Changes from all commits
0c9b24b
1628221
5598aaf
5a60199
17f5b5d
d72bd91
de1120b
47ba148
6341b5c
373af76
79f8c51
b16735c
5136357
f76f5e6
e7a4056
ddfe943
54cbd92
8d0c007
bc7c228
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,26 +17,32 @@ function getCurrentHash() { | |
|
||
return ''; | ||
} | ||
/** | ||
* | ||
* @param {Object} options | ||
* @param {string} options.screenerApiKey | ||
* @param {string} options.sourceBranchName | ||
* @param {string} options.deployUrl | ||
* @param {string} options.targetBranch | ||
* @returns | ||
*/ | ||
function getConfig({ screenerApiKey, sourceBranchName, deployUrl, targetBranch }) { | ||
const baseBranch = targetBranch ? targetBranch.replace(/^refs\/heads\//, '') : 'master'; | ||
// https://github.com/screener-io/screener-storybook#additional-configuration-options | ||
const config = { | ||
projectRepo: 'microsoft/fluentui/react-components', | ||
storybookStaticBuildDir: 'dist/storybook', | ||
storybookConfigDir: '.storybook', | ||
apiKey: screenerApiKey, | ||
resolution: '1024x768', | ||
baseBranch, | ||
failureExitCode: 0, | ||
alwaysAcceptBaseBranch: true, | ||
...(sourceBranchName !== 'master' ? { commit: getCurrentHash() } : null), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there is duplication in every config defined, can we make it DRY ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the configs for v8 and v9 are indeed almost identical, will check if it's possible to use only one file There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. they should live within scripts/screener domain, later on this will be separate package There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will make this change in the same follow-up PR as the one mentioned here as this PR only deals with changes regarding env variables: https://github.com/microsoft/fluentui/pull/24739/files#r969406141 |
||
baseUrl: `${deployUrl}/react-components-screener/iframe.html`, | ||
}; | ||
console.log('Screener config: ' + JSON.stringify({ ...config, apiKey: '...' }, null, 2)); | ||
return config; | ||
} | ||
|
||
const baseBranch = process.env.SYSTEM_PULLREQUEST_TARGETBRANCH | ||
? process.env.SYSTEM_PULLREQUEST_TARGETBRANCH.replace(/^refs\/heads\//, '') | ||
: 'master'; | ||
|
||
// https://github.com/screener-io/screener-storybook#additional-configuration-options | ||
const config = { | ||
projectRepo: 'microsoft/fluentui/react-components', | ||
storybookStaticBuildDir: 'dist/storybook', | ||
storybookConfigDir: '.storybook', | ||
apiKey: process.env.SCREENER_API_KEY, | ||
resolution: '1024x768', | ||
baseBranch, | ||
failureExitCode: 0, | ||
alwaysAcceptBaseBranch: true, | ||
...(process.env.BUILD_SOURCEBRANCH && process.env.BUILD_SOURCEBRANCH.indexOf('refs/pull') > -1 | ||
? { commit: getCurrentHash() } | ||
: null), | ||
baseUrl: `${process.env.DEPLOYURL}/react-components-screener/iframe.html`, | ||
}; | ||
console.log('Screener config: ' + JSON.stringify({ ...config, apiKey: '...' }, null, 2)); | ||
|
||
module.exports = config; | ||
module.exports = getConfig; |
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.
it would be great if we can extract this type to encapsulated interface and use it whenever needed