Add comment target flag and parsing #1241

tasdomas · 2022-10-28T14:03:27Z

One significant change in this PR is that bitbucket commit comments will no longer be automatically posted at the PR level.

The --target=auto scenario is not yet being handled in this PR, this will be implemented in a follow-up.

The general idea is as follows:

legacy comment target flags (--pr, --commitSha) take precendence.
the --issue flag will be removed in a follow-up (replaced with --target=issue#id)
the --target flag can take both qualified (--target=pr#id or --target=commit#sha or --target=issue#id) and unqualified values (--target=pr or --target=commit)
when parsing an unqualified pr target, cml will first attempt to determine the PR number using the forge context (env vars for bitbucket and gitlab, context.pull_request.number for github) - if that fails, an attempt will be made to determine the pr number using the sha of the head commit
when parsing an unqualified commit target, comments will be attached to the head commit

Add warn method. Rename commit comment methods with 'commit' prefix.

The flag is replaced with --target=issue#<id>.

--target=auto will create PR comments if the comment is run in an action with a PR context or if the HEAD commit is associated with a PR. Otherwise a commit comment will be created.

dacbd · 2022-11-03T19:05:44Z

This portion is to just add the --target=commit type functionality? and not --target=issue#1234 type correct?

tasdomas · 2022-11-03T19:07:58Z

This adds support for all target types

…

On Thu, Nov 3, 2022, 21:05 Daniel Barnes ***@***.***> wrote: This portion is to just add the --target=commit type functionality? and not --target=issue#1234 type correct? — Reply to this email directly, view it on GitHub <#1241 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAA5DNERMJLHB3FSYUSXSWLWGQEJHANCNFSM6AAAAAARRDJ6XQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

dacbd

using a --target=issue#1234 creates commit comment. see: https://github.com/iterative/cml-playground/actions/runs/3388499291/jobs/5630555691#step:5:11

github-actions · 2022-11-03T21:54:59Z

Test Comment

github-actions · 2022-11-03T21:55:02Z

Test Comment

tasdomas · 2022-11-04T08:06:27Z

using a --target=issue#1234 creates commit comment. see: https://github.com/iterative/cml-playground/actions/runs/3388499291/jobs/5630555691#step:5:11

Fixed.

DavidGOrtega

One significant change in this PR is that bitbucket commit comments will no longer be automatically posted at the PR level.

Why? It goes against the current CML definition (the video is even showing it as the rest of forges)

tasdomas · 2022-11-04T11:48:35Z

One significant change in this PR is that bitbucket commit comments will no longer be automatically posted at the PR level.

Why? It goes against the current CML definition (the video is even showing it as the rest of forges)

Because this behavior is inconsistent across forges. We don't do the same thing for github, where the support for commit comments is similar to bitbucket's. In addition to this, the side effect of the current implementation is that the PR comments are augmented with a link to the commit on all forges, which imho is unnecessary.

bin/cml/comment/create.js

casperdcl · 2022-11-08T16:07:18Z

bin/cml/comment/create.js

+    type: 'string',
+    description:
+      'Forge object to create comment on, can be one of pr, commit or issue. ' +
+      "Specify 'issue#123' to create a comment on a specific issue. " +


what syntaxes are supported?

123

#123

issue#123

pr#123

mr#123

URL

Valid targets are:

auto determine comment target automatically:

PR if running in a PR context

PR if HEAD commit is part of a PR

HEAD commit (fallback)

pr - determine pr by context or head commit (making allowances for how github's PR merge commits)

pr#123 - specific PR number

commit - current HEAD commit

commit#adfa12 - specific commit

issue#123 - specific issue

~~I think we need:~~

~~#123 to auto-infer pr-or-issue~~

~~mr alias for pr~~

~~mr#123 alias for pr#123~~

~~why do we have commit#d34db3? Doesn't --commit-sha=d34db3 already cover this?~~

~~full URL (e.g. if they want to target another PR returned by cml pr)~~

moved to #1228 (review)

github-actions · 2022-11-09T09:56:17Z

Test Comment

github-actions · 2022-11-09T09:56:21Z

Test Comment

tasdomas · 2022-11-09T10:19:43Z

One significant change in this PR is that bitbucket commit comments will no longer be automatically posted at the PR level.

Why? It goes against the current CML definition (the video is even showing it as the rest of forges)

Can we close this change request?

DavidGOrtega

👋 great effort!
I do not get the warn method.
It should be at most a CML capability and to be honest not even that. We already have a logging mechanism in place.
Its also missing in GH?

Probably I missing something 🙏

src/cml.js

src/commenttarget.js

src/drivers/bitbucket_cloud.js

src/drivers/gitlab.js

0x2b3bfa0 · 2022-11-09T13:09:56Z

I do not get the warn method.
It should be at most a CML capability and to be honest not even that. We already have a logging mechanism in place.
Its also missing in GH?

The warn method is meant to display warnings in the forge user interface, rather than just in the logs. It augments functionality supplied by winston.log but doesn't replace it. See #1227 (comment) for context.

DavidGOrtega · 2022-11-09T13:13:59Z

The warn method is meant to display warnings in the forge user interface, rather than just in the logs.

But its doing at the end a console.log so could winston handle it?

github-actions · 2022-11-09T13:37:20Z

Test Comment

github-actions · 2022-11-09T13:37:23Z

Test Comment

0x2b3bfa0 · 2022-11-09T13:43:30Z

But its doing at the end a console.log so could winston handle it?

Yes, except for GitHub, see my review suggestions above

Resolved with cacae963b9bbfe748f2c128896452a2aa32f04beb

dacbd

final testing to be done in the feature branch

* Allow creation of issue comments in cml (#1202) * Driver support for issue comments. * Create or update issue comments. * Add e2e issue comment creation tests for github and bitbucket drivers. * Add gitlab issue comment e2e test. * Add comment target flag and parsing (#1241) * Add target cli option. * Add comment target parsing. * Add debug logging in comment target parsing. Co-authored-by: DavidGOrtega <[email protected]> Co-authored-by: Casper da Costa-Luis <[email protected]>

tasdomas temporarily deployed to internal October 28, 2022 14:03 Inactive

tasdomas marked this pull request as draft October 28, 2022 14:07

tasdomas added 4 commits October 28, 2022 17:08

Add target cli option.

e70fc38

Add comment target parsing.

2324313

Update driver code.

17512f1

Add warn method. Rename commit comment methods with 'commit' prefix.

Use comment target parsing.

5b1deb4

tasdomas force-pushed the d009-comment-target branch from 3851bb2 to 5b1deb4 Compare October 28, 2022 14:08

tasdomas temporarily deployed to internal October 28, 2022 14:08 Inactive

tasdomas marked this pull request as ready for review November 3, 2022 06:08

tasdomas added 2 commits November 3, 2022 10:49

Remove --issue flag for comment create.

6b99ce9

The flag is replaced with --target=issue#<id>.

Handle --target=auto for comment create.

85281d9

--target=auto will create PR comments if the comment is run in an action with a PR context or if the HEAD commit is associated with a PR. Otherwise a commit comment will be created.

tasdomas temporarily deployed to internal November 3, 2022 09:46 Inactive

Remove default commitSha flag value.

f1f4f62

tasdomas temporarily deployed to internal November 3, 2022 10:47 Inactive

Remove comment --target flag conflicting flag list.

4e15890

tasdomas temporarily deployed to internal November 3, 2022 11:33 Inactive

Silent default for --target flag.

dc43ca0

tasdomas temporarily deployed to internal November 3, 2022 17:43 Inactive

dacbd previously requested changes Nov 3, 2022

View reviewed changes

Fix passing of commit flag value to comment target.

acae963

tasdomas temporarily deployed to internal November 3, 2022 21:52 Inactive

DavidGOrtega suggested changes Nov 4, 2022

View reviewed changes

tasdomas mentioned this pull request Nov 4, 2022

Feature comment target #1228

Merged

4 tasks

Merge branch 'feature-comment-target' into d009-comment-target

edc1b7c

This comment was marked as resolved.

Sign in to view

casperdcl reviewed Nov 8, 2022

View reviewed changes

bin/cml/comment/create.js Outdated Show resolved Hide resolved

casperdcl reviewed Nov 8, 2022

View reviewed changes

tasdomas added 2 commits November 9, 2022 11:43

Fix comment create cli param definition.

c0d9761

Fix handling of commit comments.

5d2785a

tasdomas temporarily deployed to internal November 9, 2022 09:53 Inactive

tasdomas self-assigned this Nov 9, 2022

tasdomas requested review from DavidGOrtega, dacbd and 0x2b3bfa0 November 9, 2022 10:20

0x2b3bfa0 changed the title ~~Add comment target flag and parsing.~~ Add comment target flag and parsing Nov 9, 2022

DavidGOrtega suggested changes Nov 9, 2022

View reviewed changes

src/cml.js Outdated Show resolved Hide resolved

src/cml.js Show resolved Hide resolved

src/commenttarget.js Outdated Show resolved Hide resolved

src/drivers/bitbucket_cloud.js Outdated Show resolved Hide resolved

src/drivers/gitlab.js Outdated Show resolved Hide resolved

Address review comments.

e7a7996

tasdomas temporarily deployed to internal November 9, 2022 13:34 Inactive

DavidGOrtega approved these changes Nov 9, 2022

View reviewed changes

0x2b3bfa0 approved these changes Nov 9, 2022

View reviewed changes

dacbd approved these changes Nov 9, 2022

View reviewed changes

tasdomas merged commit dd911b1 into feature-comment-target Nov 9, 2022

tasdomas deleted the d009-comment-target branch November 9, 2022 17:39

dacbd mentioned this pull request Feb 17, 2023

--append to reports #803

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add comment target flag and parsing #1241

Add comment target flag and parsing #1241

tasdomas commented Oct 28, 2022 •

edited

Loading

dacbd commented Nov 3, 2022

tasdomas commented Nov 3, 2022 via email

dacbd left a comment

github-actions bot commented Nov 3, 2022

github-actions bot commented Nov 3, 2022

tasdomas commented Nov 4, 2022

DavidGOrtega left a comment

tasdomas commented Nov 4, 2022 •

edited by dacbd

Loading

This comment was marked as resolved.

casperdcl Nov 8, 2022 •

edited

Loading

tasdomas Nov 9, 2022

casperdcl Nov 16, 2022 •

edited

Loading

github-actions bot commented Nov 9, 2022

github-actions bot commented Nov 9, 2022

tasdomas commented Nov 9, 2022

DavidGOrtega left a comment •

edited

Loading

0x2b3bfa0 commented Nov 9, 2022

DavidGOrtega commented Nov 9, 2022 •

edited

Loading

github-actions bot commented Nov 9, 2022

github-actions bot commented Nov 9, 2022

0x2b3bfa0 commented Nov 9, 2022

dacbd left a comment

Add comment target flag and parsing #1241

Add comment target flag and parsing #1241

Conversation

tasdomas commented Oct 28, 2022 • edited Loading

dacbd commented Nov 3, 2022

tasdomas commented Nov 3, 2022 via email

dacbd left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 3, 2022

Test Comment

github-actions bot commented Nov 3, 2022

Test Comment

tasdomas commented Nov 4, 2022

DavidGOrtega left a comment

Choose a reason for hiding this comment

tasdomas commented Nov 4, 2022 • edited by dacbd Loading

This comment was marked as resolved.

casperdcl Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

tasdomas Nov 9, 2022

Choose a reason for hiding this comment

casperdcl Nov 16, 2022 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Nov 9, 2022

Test Comment

github-actions bot commented Nov 9, 2022

Test Comment

tasdomas commented Nov 9, 2022

DavidGOrtega left a comment • edited Loading

Choose a reason for hiding this comment

0x2b3bfa0 commented Nov 9, 2022

DavidGOrtega commented Nov 9, 2022 • edited Loading

github-actions bot commented Nov 9, 2022

Test Comment

github-actions bot commented Nov 9, 2022

Test Comment

0x2b3bfa0 commented Nov 9, 2022

dacbd left a comment

Choose a reason for hiding this comment

tasdomas commented Oct 28, 2022 •

edited

Loading

tasdomas commented Nov 4, 2022 •

edited by dacbd

Loading

casperdcl Nov 8, 2022 •

edited

Loading

casperdcl Nov 16, 2022 •

edited

Loading

DavidGOrtega left a comment •

edited

Loading

DavidGOrtega commented Nov 9, 2022 •

edited

Loading