Skip to content

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! 👍

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! 👍

@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