-
Notifications
You must be signed in to change notification settings - Fork 343
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
Allow creation of issue comments in cml #1202
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: {
type: 'number',
description: 'Post to the given issue number',
conflicts: ['pr', 'commitSha']
},
conflicts with commitSha
default value.
https://github.com/iterative/cml-playground/actions/runs/3187003743/jobs/5198152076
Since commitSha has a default value ('HEAD'), it is treated as always set.
Updated - thanks! |
@@ -163,6 +163,7 @@ class CML { | |||
const triggerSha = await this.triggerSha(); | |||
const { | |||
commitSha: inCommitSha = triggerSha, | |||
issue: issueId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bugging me as potential source of lots of future dev headache.
It's technically issueNumber
, not issueId
, right?
gh issue view $NUMBER --json id --jq .id
!= $NUMBER
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
github is not the only forge we support and on bitbucket and gitlab issue id is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the flag is still just --issue
I don't feel this variable name that's not exposed to the user is a concern
e2bed3a
to
fe9a06f
Compare
@tasdomas are you going to open a draft PR for https://github.com/iterative/cml/tree/feature-comment-target? |
@casperdcl I was planning to submit smaller PRs into the feature branch and then propose landing it in master once its done. |
May as well open a draft PR as well? Makes it easier to track (at least for me) |
* 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]>
Closes #1180.