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

Integrate cml publish with cml send-comment #1026

Merged
merged 23 commits into from
Jun 23, 2022
Merged

Integrate cml publish with cml send-comment #1026

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
bcaaaeb
Integrate `cml publish` with `cml send-comment`
0x2b3bfa0 May 27, 2022
36b6c6b
Update snapshot tests
0x2b3bfa0 May 27, 2022
afe8be5
Merge branch 'master' into comment-publish-report
0x2b3bfa0 May 29, 2022
869ad75
Add watcher
0x2b3bfa0 May 29, 2022
8325f92
Use session seeds
0x2b3bfa0 Jun 1, 2022
c18317e
Cleanup package.json
0x2b3bfa0 Jun 1, 2022
448f0cf
Remove report.md
0x2b3bfa0 Jun 1, 2022
f416c11
Merge branch 'master' into comment-publish-report
0x2b3bfa0 Jun 3, 2022
4701660
Merge branch 'master' into comment-publish-report
0x2b3bfa0 Jun 7, 2022
44e3826
Add file-based synchronization mechanism
0x2b3bfa0 Jun 8, 2022
ac54496
Polish code
0x2b3bfa0 Jun 8, 2022
4c21ff5
Prevent `@npcz/magic` from changing access times
0x2b3bfa0 Jun 8, 2022
ab28d8d
Enable `chokidar` stabilizer
0x2b3bfa0 Jun 8, 2022
5fba6fc
Remove unnecessary guard and reduce stability interval
0x2b3bfa0 Jun 8, 2022
e9465da
Fix tests
0x2b3bfa0 Jun 8, 2022
a54bd35
Use wait-forever package
0x2b3bfa0 Jun 9, 2022
362d9f9
Apply code review suggestions
0x2b3bfa0 Jun 12, 2022
2ea74c7
Merge branch 'master' into comment-publish-report
0x2b3bfa0 Jun 12, 2022
e78bbec
Remove unused package
0x2b3bfa0 Jun 12, 2022
484143d
Use older versions of packages to avoid ESM
0x2b3bfa0 Jun 14, 2022
ca2621a
Merge branch 'master' into comment-publish-report
0x2b3bfa0 Jun 14, 2022
303fe2a
Merge branch 'master' into comment-publish-report
DavidGOrtega Jun 15, 2022
c0730c6
Merge branch 'master' into comment-publish-report
0x2b3bfa0 Jun 23, 2022
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
25 changes: 21 additions & 4 deletions bin/cml/send-comment.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
const fs = require('fs').promises;
const kebabcaseKeys = require('kebabcase-keys');

const CML = require('../../src/cml').default;
Expand All @@ -7,10 +6,9 @@ exports.command = 'send-comment <markdown file>';
exports.description = 'Comment on a commit';

exports.handler = async (opts) => {
const path = opts.markdownfile;
const report = await fs.readFile(path, 'utf-8');
opts.markdownFile = opts.markdownfile;
const cml = new CML(opts);
console.log(await cml.commentCreate({ ...opts, report }));
console.log(await cml.commentCreate(opts));
};

exports.builder = (yargs) =>
Expand All @@ -27,6 +25,25 @@ exports.builder = (yargs) =>
default: 'HEAD',
description: 'Commit SHA linked to this comment'
},
publish: {
type: 'boolean',
description:
'Upload local files and images linked from the Markdown report'
},
watch: {
type: 'boolean',
casperdcl marked this conversation as resolved.
Show resolved Hide resolved
description: 'Watch for changes and automatically update the report'
},
triggerFile: {
casperdcl marked this conversation as resolved.
Show resolved Hide resolved
type: 'string',
description: 'File used to trigger the watcher',
Copy link
Contributor

Choose a reason for hiding this comment

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

Trigger how?

Suggested change
description: 'File used to trigger the watcher',
description: 'If specified, --watch will trigger only when the lockfile is present, and will delete the lockfile',

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly as stated in the “using a trigger file” section on #1026 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

It's hard to describe in a help text. 😅

  • Takes a path to a regular file that doesn't exist yet
  • Creating a regular file in that path triggers an --update
  • Once the --update finishes, the file gets automatically deleted

Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
description: 'File used to trigger the watcher',
description: 'Path to the watcher trigger; create a file on that path to triger an update, then wait until the file disappears',

Copy link
Contributor

Choose a reason for hiding this comment

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

Need to do #762 before rewording this again :)

Copy link
Contributor

@casperdcl casperdcl Jun 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

update: If you'd prefer to do #762 immediately after this PR please do feel free to leave this unresolved here @0x2b3bfa0

Copy link
Member Author

Choose a reason for hiding this comment

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

If you're happy to see #1073 merged immediately after this pull request, fine by me. I would push a new commit to #1073 with unified periods and (perhaps) some better descriptions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Given that this is a hidden option and we don't know if DVCLive is going to use it, I'd rather not care too much about rewording (?)

Copy link
Contributor

Choose a reason for hiding this comment

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

resolving unresolved

hidden: true
},
native: {
type: 'boolean',
description:
"Uses driver's native capabilities to upload assets instead of CML's storage. Not available on GitHub."
0x2b3bfa0 marked this conversation as resolved.
Show resolved Hide resolved
},
update: {
type: 'boolean',
description:
Expand Down
7 changes: 7 additions & 0 deletions bin/cml/send-comment.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,13 @@ describe('Comment integration tests', () => {
specified commit [boolean]
--commit-sha, --head-sha Commit SHA linked to this comment
[string] [default: \\"HEAD\\"]
--publish Upload local files and images linked from the
Markdown report [boolean]
--watch Watch for changes and automatically update the
report [boolean]
--native Uses driver's native capabilities to upload assets
instead of CML's storage. Not available on GitHub.
[boolean]
--update Update the last CML comment (if any) instead of
creating a new one [boolean]
--rm-watermark Avoid watermark. CML needs a watermark to be able to
Expand Down
Loading