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

Fix send comment pr error without PR #791

Merged
merged 4 commits into from
Oct 29, 2021
Merged

Fix send comment pr error without PR #791

merged 4 commits into from
Oct 29, 2021

Conversation

DavidGOrtega
Copy link
Contributor

@DavidGOrtega DavidGOrtega commented Oct 29, 2021 •
edited
Loading

@DavidGOrtega DavidGOrtega temporarily deployed to internal October 29, 2021 09:56 Inactive
@DavidGOrtega DavidGOrtega mentioned this pull request Oct 29, 2021
Closed
@DavidGOrtega DavidGOrtega mentioned this pull request Oct 29, 2021
Merged
casperdcl
casperdcl previously approved these changes Oct 29, 2021
View reviewed changes
@casperdcl casperdcl added bug Something isn't working cml-pr Subcommand cml-comment Subcommand ui/ux User interface/experience labels Oct 29, 2021
@casperdcl casperdcl self-requested a review October 29, 2021 11:25
@casperdcl
Copy link
Contributor

casperdcl commented Oct 29, 2021 •
edited
Loading

summary: technically there are 3 on.pull_request use cases:

  1. cml send-comment --pr [--commit-sha=$GITHUB_SHA] (correctly posts to existing PR)
  2. cml pr -> cml send-comment --pr --commit-sha=$GITHUB_SHA posts to existing PR
  3. cml pr -> cml send-comment --pr --commit-sha=HEAD posts to newly created PR

and our only debate is whether to make (2) or (3) default for --commit-sha. Seems more consistent to leave (2) as default & document this.

@DavidGOrtega
Copy link
Contributor Author

cml pr -> cml send-comment --pr --commit-sha $GITHUB_SHA posts to existing PR

does not even need the explicit --commit-sha $GITHUB_SHA as without --pr behaves

@DavidGOrtega DavidGOrtega temporarily deployed to internal October 29, 2021 11:34 Inactive
@casperdcl casperdcl self-assigned this Oct 29, 2021
@DavidGOrtega DavidGOrtega self-assigned this Oct 29, 2021
@DavidGOrtega
Copy link
Contributor Author

@casperdcl is ready

casperdcl added a commit to iterative/cml.dev that referenced this pull request Oct 29, 2021
@casperdcl
pr & send-comment usage
0cdc89e 
- follow-up to iterative/cml#791
@casperdcl casperdcl mentioned this pull request Oct 29, 2021
Merged
@DavidGOrtega DavidGOrtega removed their assignment Oct 29, 2021
DavidGOrtega
DavidGOrtega commented Oct 29, 2021
View reviewed changes
Copy link
Contributor Author

@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.

Feel free to approve and merge @casperdcl

@casperdcl casperdcl temporarily deployed to internal October 29, 2021 12:38 Inactive
casperdcl
casperdcl approved these changes Oct 29, 2021
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.

just added a commit @DavidGOrtega, lemme know if it seems sensible

casperdcl
casperdcl reviewed Oct 29, 2021
View reviewed changes
@DavidGOrtega DavidGOrtega merged commit a2b21eb into master Oct 29, 2021
@DavidGOrtega DavidGOrtega deleted the send-comment-pr-error-no-PR branch October 29, 2021 12:47
@casperdcl casperdcl mentioned this pull request Oct 29, 2021
Closed
@snyk-bot snyk-bot mentioned this pull request May 29, 2022
Merged
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

@0x2b3bfa0 0x2b3bfa0 Awaiting requested review from 0x2b3bfa0

Assignees

@DavidGOrtega DavidGOrtega

@casperdcl casperdcl

Labels
bug Something isn't working cml-comment Subcommand cml-pr Subcommand ui/ux User interface/experience
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Error when combining cml pr and cml send-comment --pr
2 participants
@DavidGOrtega @casperdcl