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.

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

github(summary): build record upload optional #394

Merged
merged 1 commit into from
Jul 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
46 changes: 46 additions & 0 deletions __tests__/github.test.itg.ts
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,52 @@ maybe('writeBuildSummary', () => {
}
});
});

it('without build record', async () => {
const startedTime = new Date();
const buildx = new Buildx();
const build = new Build({buildx: buildx});

fs.mkdirSync(tmpDir, {recursive: true});
await expect(
(async () => {
// prettier-ignore
const buildCmd = await buildx.getCommand([
'--builder', process.env.CTN_BUILDER_NAME ?? 'default',
'build',
'-f', path.join(fixturesDir, 'hello.Dockerfile'),
fixturesDir,
'--metadata-file', build.getMetadataFilePath()
]);
await Exec.exec(buildCmd.command, buildCmd.args);
})()
).resolves.not.toThrow();

const refs = Buildx.refs({
dir: Buildx.refsDir,
builderName: process.env.CTN_BUILDER_NAME ?? 'default',
since: startedTime
});
expect(refs).toBeDefined();
expect(Object.keys(refs).length).toBeGreaterThan(0);

const history = new History({buildx: buildx});
const exportRes = await history.export({
refs: [Object.keys(refs)[0] ?? '']
});
expect(exportRes).toBeDefined();
expect(exportRes?.dockerbuildFilename).toBeDefined();
expect(exportRes?.dockerbuildSize).toBeDefined();
expect(exportRes?.summaries).toBeDefined();

await GitHub.writeBuildSummary({
exportRes: exportRes,
inputs: {
context: fixturesDir,
file: path.join(fixturesDir, 'hello.Dockerfile')
}
});
});
});

maybe('annotateBuildWarnings', () => {
Expand Down
43 changes: 26 additions & 17 deletions src/github.ts
Original file line number Diff line number Diff line change
Expand Up @@ -226,30 +226,39 @@ export class GitHub {

const refsSize = Object.keys(opts.exportRes.refs).length;

// we just need the last two parts of the URL as they are always relative
// to the workflow run URL otherwise URL could be broken if GitHub
// repository name is part of a secret value used in the workflow. e.g.:
// artifact: https://github.com/docker/actions-toolkit/actions/runs/9552208295/artifacts/1609622746
// workflow: https://github.com/docker/actions-toolkit/actions/runs/9552208295
// https://github.com/docker/actions-toolkit/issues/367
const artifactRelativeURL = `./${GitHub.runId}/${opts.uploadRes.url.split('/').slice(-2).join('/')}`;

// prettier-ignore
const sum = core.summary
.addHeading('Docker Build summary', 2)
.addRaw(`<p>`)
const sum = core.summary.addHeading('Docker Build summary', 2);

if (opts.uploadRes) {
// we just need the last two parts of the URL as they are always relative
// to the workflow run URL otherwise URL could be broken if GitHub
// repository name is part of a secret value used in the workflow. e.g.:
// artifact: https://github.com/docker/actions-toolkit/actions/runs/9552208295/artifacts/1609622746
// workflow: https://github.com/docker/actions-toolkit/actions/runs/9552208295
// https://github.com/docker/actions-toolkit/issues/367
const artifactRelativeURL = `./${GitHub.runId}/${opts.uploadRes.url.split('/').slice(-2).join('/')}`;

// prettier-ignore
sum.addRaw(`<p>`)
.addRaw(`For a detailed look at the build, download the following build record archive and import it into Docker Desktop's Builds view. `)
.addBreak()
.addRaw(`Build records include details such as timing, dependencies, results, logs, traces, and other information about a build. `)
.addRaw(addLink('Learn more', 'https://www.docker.com/blog/new-beta-feature-deep-dive-into-github-actions-docker-builds-with-docker-desktop/?utm_source=github&utm_medium=actions'))
.addRaw('</p>')
.addRaw(`<p>`)
.addRaw(`:arrow_down: ${addLink(`<strong>${Util.stringToUnicodeEntities(opts.uploadRes.filename)}</strong>`, artifactRelativeURL)} (${Util.formatFileSize(opts.uploadRes.size)} - includes <strong>${refsSize} build record${refsSize > 1 ? 's' : ''}</strong>)`)
.addRaw(`</p>`)
.addRaw(`<p>`)
.addRaw(`Find this useful? `)
.addRaw(addLink('Let us know', 'https://docs.docker.com/feedback/gha-build-summary'))
.addRaw('</p>');
.addRaw(`</p>`);
} else {
// prettier-ignore
sum.addRaw(`<p>`)
.addRaw(`The following table provides a brief summary of your build.`)
.addBreak()
.addRaw(`For a detailed look at the build, including timing, dependencies, results, logs, traces, and other information, consider enabling the export of the build record so you can import it into Docker Desktop's Builds view. `)
.addRaw(addLink('Learn more', 'https://www.docker.com/blog/new-beta-feature-deep-dive-into-github-actions-docker-builds-with-docker-desktop/?utm_source=github&utm_medium=actions'))
.addRaw(`</p>`);
}

// Feedback survey
sum.addRaw(`<p>`).addRaw(`Find this useful? `).addRaw(addLink('Let us know', 'https://docs.docker.com/feedback/gha-build-summary')).addRaw('</p>');
Comment on lines +260 to +261
Copy link

Choose a reason for hiding this comment

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

What if we moved this to the end? The inputs and bake def are collapsed by default, so it should still be visible. There's something odd about having this question appear right before the summary.

Copy link
Member Author

@crazy-max crazy-max Jul 2, 2024

Choose a reason for hiding this comment

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

Agreed but needs to consider the table can be quite huge if building many targets with bake like

image

Copy link

Choose a reason for hiding this comment

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

Yes, but this one is a bit of a special case, right? Do people tend to build more than ~5 or so targets in a run?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes indeed, WDYT @colinhemmings?

Choose a reason for hiding this comment

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

I would like @sharonchang1 input, but I feel like to help discovery it should be close to the top. Users are likely to expand Inputs and definitions to see what they look like, pushing the link down the page.

Copy link
Member Author

Choose a reason for hiding this comment

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

@colinhemmings Agreed, maybe right after the table then? We can look at this as follow-up as well.

Copy link

Choose a reason for hiding this comment

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

if they expand the sections and look at the summary, would they scroll back up later to give feedback?


// Preview
sum.addRaw('<p>');
Expand Down
2 changes: 1 addition & 1 deletion src/types/github.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ export interface UploadArtifactResponse {

export interface BuildSummaryOpts {
exportRes: ExportRecordResponse;
uploadRes: UploadArtifactResponse;
uploadRes?: UploadArtifactResponse;
// eslint-disable-next-line @typescript-eslint/no-explicit-any
inputs?: any;
bakeDefinition?: BakeDefinition;
Expand Down
Loading