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.

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

Add commit API endpoint #74

Merged
merged 1 commit into from
Feb 19, 2022
Merged

Add commit API endpoint #74

merged 1 commit into from
Feb 19, 2022

Conversation

dinooo13
Copy link
Contributor

@dinooo13 dinooo13 commented Nov 2, 2021

Hi @clue this is my first try at a contribution, I hope it's any good.

@dinooo13
Copy link
Contributor Author

friendly reminder 😄

Copy link
Owner

@clue clue left a comment

Choose a reason for hiding this comment

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

@dinooo13 Thank you for looking into this, very nice contribution! Agree that this feature makes perfect sense! (closes #16)

I've added some minor remarks below, otherwise LGTM. Can you squash all your changes once done?

Thank you again, keep it up! 👍

src/Client.php Outdated Show resolved Hide resolved
tests/ClientTest.php Outdated Show resolved Hide resolved
Copy link
Contributor

@SimonFrings SimonFrings left a comment

Choose a reason for hiding this comment

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

Nice work 👍

@SimonFrings SimonFrings requested a review from clue February 14, 2022 10:09
Copy link
Owner

@clue clue left a comment

Choose a reason for hiding this comment

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

@dinooo13 Thank you for looking into this and providing this high-quality PR, much appreciated! 👍

The containerCommit() API name is interesting, given the API docs use "ImageCommit" ("Create a new image from a container") and the API endpoint and docker CLI command are both named just "commit". What do you think about this?

Also noticed two minor issues below, can you look into this?

Keep it up! 👍

src/Client.php Outdated Show resolved Hide resolved
@clue clue added this to the v1.4.0 milestone Feb 19, 2022
Copy link
Owner

@clue clue left a comment

Choose a reason for hiding this comment

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

@dinooo13 Thank you very much for the update, changes LGTM! :shipit: Keep it up! 👍

@clue clue merged commit d55d009 into clue:master Feb 19, 2022
@clue clue mentioned this pull request Feb 19, 2022
@dinooo13 dinooo13 deleted the commit branch September 30, 2022 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants