Skip to content

Allow STOPSIGNAL instruction in commit change#43369

Merged
thaJeztah merged 1 commit intomoby:masterfrom
sestegra:stop
Mar 26, 2022
Merged

Allow STOPSIGNAL instruction in commit change#43369
thaJeztah merged 1 commit intomoby:masterfrom
sestegra:stop

Conversation

@sestegra
Copy link
Copy Markdown
Contributor

@sestegra sestegra commented Mar 12, 2022

- What I did
Add stopsignal in validCommitCommands for commit command.

- How I did it

  • docker commit command requests CreateImageFromContainer function.
    func (daemon *Daemon) CreateImageFromContainer(name string, c *backend.CreateImageConfig) (string, error) {
  • This function calls dockerfile.BuildFromConfig function
    newConfig, err := dockerfile.BuildFromConfig(c.Config, c.Changes, container.OS)
  • validCommitCommands is used to rejected unsupported instructions in dockerfile.BuildFromConfig
    if !validCommitCommands[n.Value] {
  • Back to CreateImageFromContainer function, merge function already support StopSIgnal
    if userConf.StopSignal == "" {

- How to verify it
Commit a new image with following command: docker commit xxxx -c "STOPSIGNAL SIGRTMIN+3"

- Description for the changelog

Allow STOPSIGNAL instruction in commit changes

- A picture of a cute animal (not mandatory but encouraged)
dolphin

Signed-off-by: Stéphane Este-Gracias <sestegra@gmail.com>
@sestegra sestegra requested a review from tonistiigi as a code owner March 12, 2022 16:31
@thaJeztah thaJeztah added status/2-code-review kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. impact/changelog impact/documentation labels Mar 16, 2022
@thaJeztah thaJeztah added this to the 21.xx milestone Mar 25, 2022
@thaJeztah
Copy link
Copy Markdown
Member

I was wondering recently what the future should be of this feature, or more specifically; using the Dockerfile syntax to make changes to the image configuration. It always was a bit of a hack, and wondering if we should consider using actual options for the changes to commit (docker commit --stop-signal=X --label=foo); let me try to write down things a bit in a ticket.

This PR looks fine to merge though for now (it doesn't really dig us in too much deeper into the hack); I kicked CI again as it was failing on unrelated things.

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Copy Markdown
Member

All green now; let's get this merged (thanks!)

@thaJeztah thaJeztah merged commit 1ad9a09 into moby:master Mar 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs/revisit impact/changelog impact/documentation kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. status/4-merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants