-
Notifications
You must be signed in to change notification settings - Fork 18
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
feature(Improve actions testing) #240
base: main
Are you sure you want to change the base?
Conversation
# Conflicts: # dist/index.js # dist/index.js.map
…ect-piper-action into improve-actions-testing
# Conflicts: # dist/index.js.map
src/piper.ts
Outdated
exportPipelineEnvironment: boolean | ||
} | ||
async function preparePiperPath (actionCfg: ActionConfiguration): Promise<string> { | ||
info('Preparing Piper binary path with configuration '.concat(JSON.stringify(actionCfg))) |
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'd turn this into debug log since users usually don't need such information in the logs.
src/piper.ts
Outdated
enterpriseApi = process.env.GITHUB_API_URL | ||
if (isEnterpriseStep(actionCfg.stepName)) { | ||
info('Preparing Piper binary for enterprise step') | ||
// devel:ContinuousDelivery:piper-library:ff8df33b8ab17c19e9f4c48472828ed809d4496a |
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.
please remove or replace ContinuousDelivery:piper-library, since it's SAP's internals and shouldn't be in open source part
src/github.ts
Outdated
let piper = 'piper' | ||
if (isEnterpriseStep) { | ||
piper = 'sap-piper' | ||
export function parseDevVersion (version: string): { owner: string, repository: string, commitISH: string } { |
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.
Considering that parseDevVersion
isn't directly related to GitHub, it might be more appropriate to relocate it to build.ts
.
src/build.ts
Outdated
info(`Listing cwd: ${cwd()}`) | ||
listFilesAndFolders(cwd()) | ||
|
||
info(`Listing $path: ${path}`) | ||
listFilesAndFolders(path) |
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 need this logging?
src/config.ts
Outdated
exportPipelineEnvironment: getValue('export-pipeline-environment') === 'true' | ||
} | ||
} | ||
|
||
export async function getDefaultConfig (server: string, apiURL: string, version: string, token: string, owner: string, repository: string, customDefaultsPaths: string): Promise<number> { |
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.
seems like getDefaultConfig
function return value is not used. We can make the return type Promise<void>
and use simple return
instead of return 0
} | ||
} | ||
|
||
export async function downloadDefaultConfig (server: string, apiURL: string, version: string, token: string, owner: string, repository: string, customDefaultsPaths: string): Promise<UploadResponse> { | ||
let defaultsPaths: string[] = [] | ||
|
||
// version: devel:..... | ||
if (version.startsWith('devel:')) { |
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'd add comment to clarify this condition:
// Since defaults file is located in release assets, we will take it from latest release
src/config.ts
Outdated
@@ -150,6 +252,7 @@ export async function checkIfStepActive (stepName: string, stageName: string, ou | |||
return result.exitCode | |||
} | |||
|
|||
// ? |
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.
?? 😃
src/enterprise.ts
Outdated
if (version.startsWith('devel:')) { | ||
debug(`version starts with "devel:" => ${version}`) | ||
debug(`params: ${owner}, ${repository}, ${version}, ${filename}`) | ||
listFilesAndFolders(process.cwd()) |
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.
was it for debugging?
src/enterprise.ts
Outdated
debug(`version starts with "devel:" => ${version}`) | ||
debug(`params: ${owner}, ${repository}, ${version}, ${filename}`) | ||
listFilesAndFolders(process.cwd()) | ||
version = '' |
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 suppose it should be latest
here as well?
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.
either way its gets turned to latest in the getTag function but I changed it
src/download.ts
Outdated
|
||
return piperPath | ||
} | ||
async function getPiperDownloadURL (piper: string, version?: string): Promise<string> { |
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 thnik we can safely turn version?
into version
, since there is no sense in expecting undefined
here. Same applies to getTag
function, where version: string | undefined
can be turned into version: string
Please change the title of the PR to not include SAP internals. |
Description
Jira: https://jira.tools.sap/browse/HSPIPER-425
Feature to enable the usage of develop branches of inner source of piper for testing