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

Allow argument-less cml pr (Revert of the Revert) #1277

Merged
merged 34 commits into from
Dec 20, 2022
Merged

Allow argument-less cml pr (Revert of the Revert) #1277

merged 34 commits into from
Dec 20, 2022

Conversation

dacbd
Copy link
Contributor

@dacbd dacbd commented Dec 6, 2022 •
edited by casperdcl
Loading

Previously seen in: #1263 #1270
Closes: #1203
Context: #1181

@0x2b3bfa0 can you confirm the intended use cases?

  • should a plain cml pr create infer a cml pr create .
    • no, cml pr create shouldn't try to git add .
  • a git commit -am "msg" followed by a plain cml pr create should work? pr without arguments #1203

https://github.com/iterative/cml-playground/actions/workflows/cml-1263.yml

@dacbd dacbd temporarily deployed to internal December 6, 2022 17:50 Inactive
@0x2b3bfa0
Copy link
Member

No, cml pr create isn't equivalent to cml pr create . because the latter implies git add . && git commit ... but the former doesn't.

@0x2b3bfa0
Copy link
Member

a git commit -am "msg" followed by a plain cml pr create should work?

Yes, it should work.

@dacbd
Copy link
Contributor Author

dacbd commented Dec 6, 2022

https://github.com/iterative/cml-playground/actions/runs/3632111594/jobs/6127577826#step:7:8 it seems it doesn't

@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal December 6, 2022 22:22 Inactive
@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal December 6, 2022 22:23 Inactive
@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal December 6, 2022 22:25 Inactive
@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal December 6, 2022 22:28 Inactive
@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal December 6, 2022 22:29 Inactive
@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal December 6, 2022 22:34 Inactive
@dacbd
Copy link
Contributor Author

dacbd commented Dec 6, 2022

sanity test passes.

@dacbd dacbd marked this pull request as ready for review December 6, 2022 22:48
@dacbd dacbd requested a review from 0x2b3bfa0 December 6, 2022 22:48
0x2b3bfa0
0x2b3bfa0 previously approved these changes Dec 6, 2022
View reviewed changes
Copy link
Member

@0x2b3bfa0 0x2b3bfa0 left a comment

Choose a reason for hiding this comment

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

Low-quality approval: my sanity is of no value in this case.

@dacbd dacbd temporarily deployed to internal December 6, 2022 22:51 — with GitHub Actions Inactive
@dacbd dacbd requested a review from 0x2b3bfa0 December 6, 2022 22:55
@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal December 6, 2022 23:33 — with GitHub Actions Inactive
@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal December 6, 2022 23:43 — with GitHub Actions Inactive
@github-actions

This comment was marked as outdated.

Sign in to view
@github-actions

This comment was marked as duplicate.

Sign in to view
@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal December 20, 2022 03:15 — with GitHub Actions Inactive
@github-actions

This comment was marked as duplicate.

Sign in to view
@github-actions

This comment was marked as outdated.

Sign in to view
0x2b3bfa0
0x2b3bfa0 previously approved these changes Dec 20, 2022
View reviewed changes
casperdcl
casperdcl reviewed Dec 20, 2022
View reviewed changes
bin/cml/pr/create.e2e.test.js Outdated Show resolved Hide resolved
bin/cml/pr/create.js Outdated Show resolved Hide resolved
src/cml.js Show resolved Hide resolved
src/cml.js Outdated Show resolved Hide resolved
src/cml.js Show resolved Hide resolved
@dacbd @casperdcl
Update bin/cml/pr/create.js
7c242eb 
Co-authored-by: Casper da Costa-Luis <[email protected]>
@dacbd dacbd temporarily deployed to internal December 20, 2022 16:01 — with GitHub Actions Inactive
@dacbd @casperdcl
Update src/cml.js
202a4c2 
Co-authored-by: Casper da Costa-Luis <[email protected]>
@dacbd dacbd temporarily deployed to internal December 20, 2022 16:02 — 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
@github-actions

This comment was marked as outdated.

Sign in to view
@github-actions

This comment was marked as outdated.

Sign in to view
@dacbd dacbd temporarily deployed to internal December 20, 2022 16:06 — with GitHub Actions Inactive
@dacbd dacbd enabled auto-merge (squash) December 20, 2022 16:07
@github-actions

This comment was marked as outdated.

Sign in to view
@github-actions

This comment was marked as outdated.

Sign in to view
@dacbd dacbd temporarily deployed to internal December 20, 2022 16:12 — with GitHub Actions Inactive
@github-actions
Copy link
Contributor

Test Comment

@github-actions
Copy link
Contributor

Test Comment

@dacbd dacbd mentioned this pull request Dec 20, 2022
Open
0x2b3bfa0
0x2b3bfa0 approved these changes Dec 20, 2022
View reviewed changes
Copy link
Member

@0x2b3bfa0 0x2b3bfa0 left a comment

Choose a reason for hiding this comment

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

D90nHuOXkAAGxtJ

@dacbd dacbd merged commit f209abd into master Dec 20, 2022
@dacbd dacbd deleted the revert-revert branch December 20, 2022 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@casperdcl casperdcl casperdcl left review comments

@0x2b3bfa0 0x2b3bfa0 0x2b3bfa0 approved these changes

@tasdomas tasdomas tasdomas left review comments

Assignees

@dacbd dacbd

Labels
external-request You asked, we did p0-critical Max priority (ASAP)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

pr without arguments
4 participants
@dacbd @0x2b3bfa0 @casperdcl @tasdomas