Skip to content

Commit b74f4d9

Browse files
committed
fix: use doc context in github comment docDigest
This commit includes more information in the current “docDigest” which is used to differentiate github diff comments for each docs that are diffed with the github action. We had the “hub” context encoded in the digest, but forgot the “branch” name. This could lead to an inconsistant comment if one tries to execute two diff commands, with two differnt branch name. Only the second diff comment would be visible on the PR (instead of two comments one for each branch). The commit also moves the “doc digest” computation inside the `Repo` class instead of the main code.
1 parent 31a112a commit b74f4d9

File tree

6 files changed

+37
-23
lines changed

6 files changed

+37
-23
lines changed

src/github.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import * as github from '@actions/github';
44
import * as io from '@actions/io';
55
import { GetResponseDataTypeFromEndpointMethod } from '@octokit/types';
66
import { GitHub } from '@actions/github/lib/utils.js';
7-
import { extractBumpDigest, fsExists } from './common.js';
7+
import { extractBumpDigest, fsExists, shaDigest } from './common.js';
88

99
// These are types which are not exposed directly by Github libs
1010
// which we need to define
@@ -24,7 +24,7 @@ export class Repo {
2424
readonly headSha?: string;
2525
private _docDigest: string;
2626

27-
constructor(docDigest: string) {
27+
constructor(doc: string, hub?: string, branch?: string) {
2828
// Fetch GitHub Action context
2929
// from GITHUB_REPOSITORY & GITHUB_EVENT_PATH
3030
const { owner, repo } = github.context.repo;
@@ -38,7 +38,7 @@ export class Repo {
3838
this.headSha = pull_request.head.sha;
3939
}
4040
this.octokit = this.getOctokit();
41-
this._docDigest = docDigest;
41+
this._docDigest = shaDigest([doc, hub, branch]);
4242
}
4343

4444
public get docDigest() {

src/main.ts

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import * as bump from 'bump-cli';
55

66
import * as diff from './diff.js';
77
import { Repo } from './github.js';
8-
import { shaDigest } from './common.js';
98

109
export async function run(): Promise<void> {
1110
try {
@@ -68,8 +67,7 @@ export async function run(): Promise<void> {
6867
await bump.Deploy.run(cliParams.concat(deployParams), oclifConfig);
6968
break;
7069
case 'diff':
71-
const docDigest = shaDigest([doc, hub]);
72-
const repo = new Repo(docDigest);
70+
const repo = new Repo(doc, hub, branch);
7371
let file1 = await repo.getBaseFile(file);
7472
let file2: string | undefined;
7573

tests/common.test.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@ test('shaDigest function', async () => {
77
expect(common.shaDigest(texts)).toBe('6adfb183a4a2c94a2f92dab5ade762a47889a5a1');
88
texts.pop();
99
expect(common.shaDigest(texts)).toBe('aaf4c61ddcc5e8a2dabede0f3b482cd9aea9434d');
10+
texts.push('');
11+
expect(common.shaDigest(texts)).toBe('aaf4c61ddcc5e8a2dabede0f3b482cd9aea9434d');
1012
});
1113

1214
test('fsExists function', async () => {

tests/diff.test.ts

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import * as github from './fixtures/github.js';
22
import * as bump from 'bump-cli';
33
import { jest } from '@jest/globals';
4+
import { shaDigest } from '../src/common.js';
45

56
// Mocks should be declared before the module being tested is imported.
67
jest.unstable_mockModule('@actions/github', () => github);
@@ -43,6 +44,7 @@ describe('diff.ts', () => {
4344
const digest = '4b81e612cafa6580f8ad3bfe9e970b2d961f58c2';
4445

4546
const repo = new Repo('hello');
47+
const docDigest = shaDigest(['hello']);
4648

4749
await diff.run(result, repo);
4850

@@ -57,7 +59,7 @@ describe('diff.ts', () => {
5759
[Preview documentation](https://bump.sh/doc/my-doc/changes/654)
5860
5961
> _Powered by [Bump.sh](https://bump.sh)_
60-
<!-- Bump.sh digest=${digest} doc=hello -->`,
62+
<!-- Bump.sh digest=${digest} doc=${docDigest} -->`,
6163
digest,
6264
);
6365
});
@@ -71,6 +73,7 @@ describe('diff.ts', () => {
7173
const digest = '3999a0fe6ad27841bd6342128f7028ab2cea1c57';
7274

7375
const repo = new Repo('hello');
76+
const docDigest = shaDigest(['hello']);
7477
await diff.run(result, repo);
7578

7679
expect(repo.createOrUpdateComment).toHaveBeenCalledWith(
@@ -81,7 +84,7 @@ No structural change, nothing to display.
8184
[Preview documentation](https://bump.sh/doc/my-doc/changes/654)
8285
8386
> _Powered by [Bump.sh](https://bump.sh)_
84-
<!-- Bump.sh digest=${digest} doc=hello -->`,
87+
<!-- Bump.sh digest=${digest} doc=${docDigest} -->`,
8588
digest,
8689
);
8790
});
@@ -98,6 +101,7 @@ No structural change, nothing to display.
98101
};
99102
const digest = '4b81e612cafa6580f8ad3bfe9e970b2d961f58c2';
100103
const repo = new Repo('hello');
104+
const docDigest = shaDigest(['hello']);
101105
await diff.run(result, repo);
102106

103107
expect(repo.createOrUpdateComment).toHaveBeenCalledWith(
@@ -111,7 +115,7 @@ No structural change, nothing to display.
111115
[Preview documentation](https://bump.sh/doc/my-doc/changes/654)
112116
113117
> _Powered by [Bump.sh](https://bump.sh)_
114-
<!-- Bump.sh digest=${digest} doc=hello -->`,
118+
<!-- Bump.sh digest=${digest} doc=${docDigest} -->`,
115119
digest,
116120
);
117121
});
@@ -128,6 +132,7 @@ No structural change, nothing to display.
128132
const digest = 'c1f04e5c83235377b88745d13dc9b1ebd3a125a8';
129133

130134
const repo = new Repo('hello');
135+
const docDigest = shaDigest(['hello']);
131136
await diff.run(result, repo);
132137

133138
expect(repo.createOrUpdateComment).toHaveBeenCalledWith(
@@ -139,7 +144,7 @@ No structural change, nothing to display.
139144
140145
141146
> _Powered by [Bump.sh](https://bump.sh)_
142-
<!-- Bump.sh digest=${digest} doc=hello -->`,
147+
<!-- Bump.sh digest=${digest} doc=${docDigest} -->`,
143148
digest,
144149
);
145150
});

tests/github.test.ts

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import * as common from './fixtures/common.js';
55
import { jest } from '@jest/globals';
66
import fixtureGithubContext from './fixtures/github-context.json';
77
import nock from 'nock';
8+
import { shaDigest } from '../src/common.js';
89

910
nock.disableNetConnect();
1011
stdout.start();
@@ -43,13 +44,14 @@ describe('github.ts', () => {
4344
common.fsExists.mockResolvedValue(true);
4445

4546
const repo = new Repo('hello');
47+
const docDigest = shaDigest(['hello']);
4648
const headFile = 'openapi.yml';
4749
const baseFile = await repo.getBaseFile('openapi.yml');
4850
const baseSha = fixtureGithubContext.payload.pull_request.base.sha;
4951
const headSha = fixtureGithubContext.payload.pull_request.head.sha;
5052
const baseBranch = '';
5153

52-
expect(repo.docDigest).toEqual('hello');
54+
expect(repo.docDigest).toEqual(docDigest);
5355
// Expect git executions
5456
expect(exec.exec.mock.calls).toEqual([
5557
['git', ['fetch', 'origin', baseSha, headSha]],
@@ -106,8 +108,9 @@ describe('github.ts', () => {
106108
test('Calls octokit to update the issue comment', async () => {
107109
const digest = 'existing-comment';
108110
const doc = 'hello';
109-
const body = `coucou\n<!-- Bump.sh digest=${digest} doc=${doc} -->`;
110-
const newBody = `New coucou\n<!-- Bump.sh digest=new-coucou doc=${doc} -->`;
111+
const docDigest = shaDigest(['hello']);
112+
const body = `coucou\n<!-- Bump.sh digest=${digest} doc=${docDigest} -->`;
113+
const newBody = `New coucou\n<!-- Bump.sh digest=new-coucou doc=${docDigest} -->`;
111114

112115
mockGithubComments([{ id: 1, body }]);
113116
mockGithubCommentUpdate(1, newBody);
@@ -133,4 +136,14 @@ describe('github.ts', () => {
133136
expect(buildRepo).toThrow('No GITHUB_TOKEN env variable available');
134137
});
135138
});
139+
140+
describe('constructor', () => {
141+
test('instanciate a github API client and a doc digest', async () => {
142+
const repo = new Repo('hello', 'my-hub', 'my-branch');
143+
const docDigest = shaDigest(['hello', 'my-hub', 'my-branch']);
144+
145+
expect(repo.docDigest).toEqual(docDigest);
146+
expect(repo.octokit).toBeDefined();
147+
});
148+
});
136149
});

tests/main.test.ts

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -257,15 +257,14 @@ describe('main.ts', () => {
257257
describe('diff command', () => {
258258
it('test action run diff correctly', async () => {
259259
const file = 'my-file-to-diff.yml';
260-
const emptyDocDigest = 'da39a3ee5e6b4b0d3255bfef95601890afd80709';
261260
const commentDigest = '1ab9a6fb70e07d910650e1895e9fc53570f99011';
262-
mockInputs({ file, command: 'diff' });
261+
mockInputs({ file, command: 'diff', doc: 'my-doc' });
263262
// Mock return value from bump-cli core diff lib
264263
bump.mockRunDiff.mockResolvedValue(diffExample);
265264

266265
await run();
267266

268-
expect(repo.Repo).toHaveBeenCalledWith(emptyDocDigest);
267+
expect(repo.Repo).toHaveBeenCalledWith('my-doc', '', '');
269268
expect(repo.mockGetBaseFile).toHaveBeenCalledWith(file);
270269
expect(repo.mockCreateOrUpdateComment).toHaveBeenCalledWith(
271270
`🚨 Breaking API change detected:
@@ -282,7 +281,7 @@ one
282281
expect(bump.mockRunDiff).toHaveBeenCalledWith(
283282
'my-file-to-diff.yml',
284283
undefined,
285-
'',
284+
'my-doc',
286285
'',
287286
'',
288287
'',
@@ -294,15 +293,14 @@ one
294293

295294
it('test action run diff on existing documentation correctly', async () => {
296295
const file = 'my-file-to-diff.yml';
297-
const docDigest = '398b995591d7e5f6676e44f06be071abe850b38e';
298296
const commentDigest = '1ab9a6fb70e07d910650e1895e9fc53570f99011';
299297
mockInputs({ file, doc: 'my-doc', command: 'diff' });
300298
// Mock return value from bump-cli core diff lib
301299
bump.mockRunDiff.mockResolvedValue(diffExample);
302300

303301
await run();
304302

305-
expect(repo.Repo).toHaveBeenCalledWith(docDigest);
303+
expect(repo.Repo).toHaveBeenCalledWith('my-doc', '', '');
306304
expect(repo.mockGetBaseFile).toHaveBeenCalledWith(file);
307305
expect(repo.mockCreateOrUpdateComment).toHaveBeenCalledWith(
308306
`🚨 Breaking API change detected:
@@ -331,13 +329,12 @@ one
331329

332330
it('test action run diff with Branch correctly', async () => {
333331
const file = 'my-file-to-diff.yml';
334-
const docDigest = 'da39a3ee5e6b4b0d3255bfef95601890afd80709';
335332
const commentDigest = '1ab9a6fb70e07d910650e1895e9fc53570f99011';
336333
mockInputs({ file, branch: 'latest', command: 'diff' });
337334

338335
await run();
339336

340-
expect(repo.Repo).toHaveBeenCalledWith(docDigest);
337+
expect(repo.Repo).toHaveBeenCalledWith('', '', 'latest');
341338
expect(repo.mockGetBaseFile).toHaveBeenCalledWith(file);
342339
expect(repo.mockCreateOrUpdateComment).toHaveBeenCalledWith(
343340
`🚨 Breaking API change detected:
@@ -366,15 +363,14 @@ one
366363

367364
it('test action run diff on PR correctly', async () => {
368365
const file = 'my-file-to-diff.yml';
369-
const docDigest = 'da39a3ee5e6b4b0d3255bfef95601890afd80709';
370366
const commentDigest = '1ab9a6fb70e07d910650e1895e9fc53570f99011';
371367
mockInputs({ file, command: 'diff' });
372368
// Mock base file from PR
373369
repo.mockGetBaseFile.mockResolvedValue('my-base-file-to-diff.yml');
374370

375371
await run();
376372

377-
expect(repo.Repo).toHaveBeenCalledWith(docDigest);
373+
expect(repo.Repo).toHaveBeenCalledWith('', '', '');
378374
expect(repo.mockCreateOrUpdateComment).toHaveBeenCalledWith(
379375
`🚨 Breaking API change detected:
380376

0 commit comments

Comments
 (0)