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

send-comment rev-parse #809

Merged
merged 14 commits into from
Nov 12, 2021
Merged

send-comment rev-parse #809

merged 14 commits into from
Nov 12, 2021

Conversation

DavidGOrtega
Copy link
Contributor

@DavidGOrtega DavidGOrtega commented Nov 9, 2021 •
edited
Loading

Surprisingly it looks like we lost the ability to parse refs long time ago!? (Everybody remembers it but send-comment predating --pr has the same issue).
Also fixes some BB issues

@DavidGOrtega DavidGOrtega temporarily deployed to internal November 9, 2021 21:46 Inactive
@DavidGOrtega DavidGOrtega temporarily deployed to internal November 9, 2021 22:30 Inactive
@DavidGOrtega DavidGOrtega temporarily deployed to internal November 9, 2021 23:08 Inactive
@DavidGOrtega DavidGOrtega temporarily deployed to internal November 9, 2021 23:10 Inactive
@DavidGOrtega DavidGOrtega temporarily deployed to internal November 9, 2021 23:29 Inactive
@DavidGOrtega DavidGOrtega temporarily deployed to internal November 9, 2021 23:36 Inactive
@DavidGOrtega DavidGOrtega temporarily deployed to internal November 10, 2021 10:29 Inactive
@DavidGOrtega DavidGOrtega temporarily deployed to internal November 10, 2021 14:24 Inactive
@DavidGOrtega DavidGOrtega temporarily deployed to internal November 10, 2021 14:38 Inactive
@DavidGOrtega DavidGOrtega temporarily deployed to internal November 10, 2021 15:05 Inactive
@DavidGOrtega DavidGOrtega temporarily deployed to internal November 10, 2021 15:27 Inactive
@DavidGOrtega DavidGOrtega temporarily deployed to internal November 10, 2021 16:03 Inactive
@DavidGOrtega
Copy link
Contributor Author

image

image

image

@DavidGOrtega DavidGOrtega temporarily deployed to internal November 11, 2021 12:14 Inactive
casperdcl
casperdcl reviewed Nov 11, 2021
View reviewed changes
Copy link
Contributor

@casperdcl casperdcl left a comment •
edited
Loading

Choose a reason for hiding this comment

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

maybe should add some tests?

src/cml.js Outdated Show resolved Hide resolved
@DavidGOrtega @casperdcl
Update message
7ac7f4d 
Co-authored-by: Casper da Costa-Luis <[email protected]>
@DavidGOrtega DavidGOrtega temporarily deployed to internal November 11, 2021 19:32 Inactive
@DavidGOrtega
Copy link
Contributor Author

DavidGOrtega commented Nov 11, 2021 •
edited
Loading

maybe should add some tests?

Im doing integration tests in fashion_mnist.
All tests are going to run against every single release.

I can add some also in our dummy repos but I would like to deprecate them in favour of a real thing

casperdcl
casperdcl approved these changes Nov 12, 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.

lgtm. let me know if you're ready for merge

@DavidGOrtega
Copy link
Contributor Author

Ready to be merged unless you want me to create the PR in dummy repos to test it

@casperdcl casperdcl added cml-comment Subcommand cml-pr Subcommand bug Something isn't working p0-critical Max priority (ASAP) labels Nov 12, 2021
@casperdcl casperdcl merged commit 30919bf into master Nov 12, 2021
@casperdcl casperdcl deleted the cml-send-comment-pr-rev-parse branch November 12, 2021 12:59
@0x2b3bfa0
Copy link
Member

Related to #880

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

Assignees

@DavidGOrtega DavidGOrtega

Labels
bug Something isn't working cml-comment Subcommand cml-pr Subcommand p0-critical Max priority (ASAP)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

cml send-comment fails in BB without existing PRs Error when combining cml pr and cml send-comment --pr
3 participants
@DavidGOrtega @0x2b3bfa0 @casperdcl