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

Jump to bottom

Feature comment target #1228

Merged
merged 30 commits into from
Dec 15, 2022
Merged

Feature comment target #1228

merged 30 commits into from
Dec 15, 2022

Conversation

tasdomas
Copy link
Contributor

@tasdomas tasdomas commented Oct 15, 2022 •
edited
Loading

tasdomas and others added 2 commits October 14, 2022 17:13
@tasdomas
Allow creation of issue comments in cml (#1202)
9603e66 
* Add command-line option for issue id.

* Driver support for issue comments.

* Create or update issue comments.

* Number as issue id.

* Fix cli test.

* Address review comments.

* Remove conflict between --issue and --commitSha.

Since commitSha has a default value ('HEAD'), it is treated as always set.

* Fix usage of octokit.

* Add e2e issue comment creation tests for github and bitbucket drivers.

* Default values for issue id in e2e tests.

* Add gitlab issue comment e2e test.
@tasdomas
Merge remote-tracking branch 'origin/master' into feature-comment-target
48a3258
@tasdomas tasdomas temporarily deployed to internal October 15, 2022 05:41 Inactive
@tasdomas tasdomas marked this pull request as draft October 15, 2022 05:41
@casperdcl casperdcl self-requested a review October 17, 2022 21:19
@casperdcl casperdcl added cml-comment Subcommand enhancement New feature or request ui/ux User interface/experience labels Oct 17, 2022
DavidGOrtega
DavidGOrtega previously requested changes Oct 17, 2022
View reviewed changes
Copy link
Contributor

@DavidGOrtega DavidGOrtega left a comment

Choose a reason for hiding this comment

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

Please consider this suggestion just to reduce some boilerplate.
If you still want to continue with the current code is still OK
🙏

src/drivers/bitbucket_cloud.js Outdated Show resolved Hide resolved
src/drivers/gitlab.js Outdated Show resolved Hide resolved
src/drivers/github.js Show resolved Hide resolved
@tasdomas tasdomas temporarily deployed to internal October 18, 2022 12:43 Inactive
@tasdomas tasdomas temporarily deployed to internal October 18, 2022 12:43 Inactive
@tasdomas tasdomas temporarily deployed to internal October 18, 2022 12:44 Inactive
@tasdomas tasdomas temporarily deployed to internal October 18, 2022 13:23 Inactive
@tasdomas tasdomas temporarily deployed to internal October 24, 2022 14:36 Inactive
@karl-richter
Copy link

karl-richter commented Oct 27, 2022 •
edited
Loading

Hi @tasdomas, do I understand it correct that this feature would allow to comment (and update comments) on a pull request given the respective pr number on the repo? I was about to open an issue as I seem to have run into a deadlock with commenting on a pr in the target repo when working with forks. Seems like this could resolve the issue.

@tasdomas
Copy link
Contributor Author

Hi @tasdomas, do I understand it correct that this feature would allow to comment (and update comments) on a pull request given the respective pr number on the repo? I was about to open an issue as I seem to have run into a deadlock with commenting on a pr in the target repo when working with forks. Seems like this could resolve the issue.

Yes, this will be one of the features this will introduce

@karl-richter
Copy link

Hi @tasdomas, do I understand it correct that this feature would allow to comment (and update comments) on a pull request given the respective pr number on the repo? I was about to open an issue as I seem to have run into a deadlock with commenting on a pr in the target repo when working with forks. Seems like this could resolve the issue.

Yes, this will be one of the features this will introduce

Sounds awesome! Do you have an eta for the pr / whats your release cycle?
Let me know if I can support with getting this shipped!

@tasdomas tasdomas marked this pull request as ready for review October 27, 2022 08:25
@tasdomas tasdomas marked this pull request as draft October 27, 2022 08:25
@tasdomas tasdomas temporarily deployed to internal October 27, 2022 09:02 Inactive
@tasdomas tasdomas temporarily deployed to internal October 28, 2022 14:08 Inactive
@casperdcl casperdcl mentioned this pull request Nov 3, 2022
Closed
@tasdomas tasdomas temporarily deployed to internal December 14, 2022 09:28 — with GitHub Actions Inactive
@github-actions

This comment was marked as outdated.

Sign in to view
@github-actions

This comment was marked as outdated.

Sign in to view
@tasdomas tasdomas temporarily deployed to internal December 15, 2022 14:26 — with GitHub Actions Inactive
@github-actions
Copy link
Contributor

Test Comment

@github-actions
Copy link
Contributor

Test Comment

@tasdomas tasdomas temporarily deployed to internal December 15, 2022 18:01 — with GitHub Actions Inactive
@github-actions
Copy link
Contributor

Test Comment

@github-actions
Copy link
Contributor

Test Comment

@tasdomas tasdomas temporarily deployed to internal December 15, 2022 19:16 — with GitHub Actions Inactive
@github-actions
Copy link
Contributor

Test Comment

@github-actions
Copy link
Contributor

Test Comment

@casperdcl
Copy link
Contributor

casperdcl commented Dec 15, 2022 •
edited
Loading

Are there any blockers for this? Too big & scary for anyone to click "merge"?

@tasdomas
Copy link
Contributor Author

Are there any blockers for this? Too big & scary for anyone to click "merge"?

was waiting for the tests to pass

@tasdomas
Copy link
Contributor Author

And I can't merge this - need an approval

casperdcl
casperdcl approved these changes Dec 15, 2022
View reviewed changes
Copy link
Contributor

@casperdcl casperdcl left a comment

Choose a reason for hiding this comment

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

what could possibly go wrong

@tasdomas tasdomas temporarily deployed to internal December 15, 2022 20:36 — with GitHub Actions Inactive
@github-actions
Copy link
Contributor

Test Comment

@github-actions
Copy link
Contributor

Test Comment

@tasdomas tasdomas merged commit 384cdb5 into master Dec 15, 2022
@tasdomas tasdomas deleted the feature-comment-target branch December 15, 2022 21:05
@github-actions
Copy link
Contributor

Test Comment

@github-actions
Copy link
Contributor

Test Comment

@0x2b3bfa0 0x2b3bfa0 mentioned this pull request Jan 12, 2023
Merged
@casperdcl casperdcl linked an issue Jan 26, 2023 that may be closed by this pull request
comment --pr=true default #1139
Closed
@casperdcl casperdcl mentioned this pull request Jan 26, 2023
Closed
@dacbd dacbd mentioned this pull request Feb 17, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@casperdcl casperdcl casperdcl approved these changes

@dacbd dacbd dacbd left review comments

@DavidGOrtega DavidGOrtega DavidGOrtega left review comments

Assignees

@tasdomas tasdomas

Labels
cml-comment Subcommand enhancement New feature or request ui/ux User interface/experience
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

comment --pr=true default
5 participants
@tasdomas @karl-richter @DavidGOrtega @casperdcl @dacbd