Skip to content
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.

Sign up for GitHub

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

improvement: make sure artifacts are always fetched #6532

Merged
merged 10 commits into from
Oct 17, 2024
Merged

improvement: make sure artifacts are always fetched #6532

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
12654a0
improvement: make sure artifacts are always fetched
10ko Oct 11, 2024
50225b7
chore: add comments
10ko Oct 11, 2024
0de2b90
improvement: always fetch artifacts in Run exec as well
10ko Oct 11, 2024
53f7374
fix: some edge cases
10ko Oct 11, 2024
0751af7
chore: comments
10ko Oct 11, 2024
b632ff8
Merge branch 'main' into improve-fetching-artifacts
10ko Oct 11, 2024
d208c38
Update core/src/plugins/exec/test.ts
10ko Oct 17, 2024
61e8c34
fix: wrong condition
10ko Oct 17, 2024
14cc1c5
Merge branch 'main' into improve-fetching-artifacts
10ko Oct 17, 2024
a7b6778
refactor: use let instead of array and avoid implicit any
stefreak Oct 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 56 additions & 33 deletions core/src/plugins/exec/run.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,45 +32,68 @@ export type ExecRun = GardenSdkActionDefinitionActionType<typeof execRun>
execRun.addHandler("run", async ({ artifactsPath, log, action, ctx }) => {
const startedAt = new Date()
const { command, env, artifacts } = action.getSpec()
const runCommandErrors: unknown[] = []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's always one element in the array, I think it's cleaner to use let here:

Suggested change
const runCommandErrors: unknown[] = []
let runCommandError: unknown | undefined

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

runCommandErrors is never reassigned, it will give you a type error if you use let.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's never reassigned because you push an element to the array, but why use an array in the first place if it'll never have more than one element?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aaah, that's what you meant. Sorry didn't understand you from your first comment. Sure thing, that makes sense.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@10ko I've pushed a commit with the change I had in mind, does it look good to you?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries :D


const commandResult = await execRunCommand({ command, action, ctx, log, env, opts: { reject: false } })

const detail = {
moduleName: action.moduleName(),
taskName: action.name,
command,
version: action.versionString(),
success: commandResult.success,
log: commandResult.outputLog,
startedAt,
completedAt: commandResult.completedAt,
let commandResult
try {
// Execute the run command
commandResult = await execRunCommand({ command, action, ctx, log, env, opts: { reject: false } })
} catch (error) {
// Store error to be thrown at the end after trying to fetch artifacts
runCommandErrors.push(error)
}

const result = {
state: runResultToActionState(detail),
detail,
outputs: {
try {
// Try to fetch artifacts
await copyArtifacts(log, artifacts, action.getBuildPath(), artifactsPath)
} catch (error) {
if (runCommandErrors.length > 0) {
// If the run command has failed and the artifact copy has failed as well, we just log a warning for
// the artifact copy error since we'll trow the run command error at the end
log.warn(`Failed to copy artifacts: ${error}`)
} else {
throw error
}
}
if (runCommandErrors.length === 0) {
const detail = {
moduleName: action.moduleName(),
taskName: action.name,
command,
version: action.versionString(),
success: commandResult.success,
log: commandResult.outputLog,
},
} as const
startedAt,
completedAt: commandResult.completedAt,
}

if (!commandResult.success) {
return result
}
const result = {
state: runResultToActionState(detail),
detail,
outputs: {
log: commandResult.outputLog,
},
} as const

if (commandResult.outputLog) {
const prefix = `Finished executing ${styles.highlight(action.key())}. Here is the full output:`
log.info(
renderMessageWithDivider({
prefix,
msg: commandResult.outputLog,
isError: !commandResult.success,
color: styles.primary,
})
)
}
if (!commandResult.success) {
return result
}

await copyArtifacts(log, artifacts, action.getBuildPath(), artifactsPath)
if (commandResult.outputLog) {
const prefix = `Finished executing ${styles.highlight(action.key())}. Here is the full output:`
log.info(
renderMessageWithDivider({
prefix,
msg: commandResult.outputLog,
isError: !commandResult.success,
color: styles.primary,
})
)
}

return result
return result
} else {
// The run command failed, so we throw the error
throw runCommandErrors[0]
}
})
89 changes: 56 additions & 33 deletions core/src/plugins/exec/test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,45 +34,68 @@ export type ExecTest = GardenSdkActionDefinitionActionType<typeof execTest>
execTest.addHandler("run", async ({ log, action, artifactsPath, ctx }) => {
const startedAt = new Date()
const { command, env, artifacts } = action.getSpec()
const runCommandErrors: unknown[] = []

const commandResult = await execRunCommand({ command, action, ctx, log, env, opts: { reject: false } })
let commandResult
try {
// Execute the test command
commandResult = await execRunCommand({ command, action, ctx, log, env, opts: { reject: false } })
} catch (error) {
// Store error to be thrown at the end after trying to fetch artifacts
runCommandErrors.push(error)
}

const detail = {
moduleName: action.moduleName(),
testName: action.name,
command,
version: action.versionString(),
success: commandResult.success,
log: commandResult.outputLog,
startedAt,
completedAt: commandResult.completedAt,
try {
// Try to fetch artifacts
await copyArtifacts(log, artifacts, action.getBuildPath(), artifactsPath)
} catch (error) {
if (runCommandErrors.length > 0) {
// If the test command has failed and the artifact copy has failed as well, we just log a warning for
// the artifact copy error since we'll trow the test command error at the end
log.warn(`Failed to copy artifacts: ${error}`)
} else {
throw error
}
}

const result = {
state: runResultToActionState(detail),
detail,
outputs: {
if (runCommandErrors.length === 0) {
10ko marked this conversation as resolved.
Show resolved Hide resolved
const detail = {
moduleName: action.moduleName(),
testName: action.name,
command,
version: action.versionString(),
success: commandResult.success,
log: commandResult.outputLog,
},
} as const
startedAt,
completedAt: commandResult.completedAt,
}

if (!commandResult.success) {
return result
}
const result = {
state: runResultToActionState(detail),
detail,
outputs: {
log: commandResult.outputLog,
},
} as const

if (commandResult.outputLog) {
const prefix = `Finished executing ${styles.highlight(action.key())}. Here is the full output:`
log.info(
renderMessageWithDivider({
prefix,
msg: commandResult.outputLog,
isError: !commandResult.success,
color: styles.primary,
})
)
}
if (!commandResult.success) {
return result
}

await copyArtifacts(log, artifacts, action.getBuildPath(), artifactsPath)

return result
if (commandResult.outputLog) {
const prefix = `Finished executing ${styles.highlight(action.key())}. Here is the full output:`
log.info(
renderMessageWithDivider({
prefix,
msg: commandResult.outputLog,
isError: !commandResult.success,
color: styles.primary,
})
)
}
return result
} else {
// The test command failed, so we throw the error
throw runCommandErrors[0]
}
})