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

Support for rollback config in compose 3.4 #397

Closed
wants to merge 100 commits into from

Conversation

alshabib
Copy link

@alshabib alshabib commented Jul 28, 2017

Fixes moby/moby#32585

- What I did

Built off #360 to provide support for rollback config in compose 3.4. ie.

  deploy:
      replicas: 1
      rollback_config:
        parallelism: 1

- How I did it

Added the key to the schema, regenerated the schema and tied it to existing UpdateConfig type.

- How to verify it

docker stack deploy -c docker-compose.yml yay

with the following stack:

version: '3.4'

services:
  busy:
    image: busybox
    deploy:
      replicas: 1
      rollback_config:
        parallelism: 1
    ports:
      - 80:80

- Description for the changelog
This pull request adds support for rollback configurations to be specified in the YAML stack file. Given that it is build off the aforementioned pull request, it supports also rollback_config.order.

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

That's actually my dog.

@codecov-io
Copy link

codecov-io commented Jul 28, 2017

Codecov Report

Merging #397 into master will increase coverage by 0.03%.
The diff coverage is 63.94%.

@@            Coverage Diff             @@
##           master     #397      +/-   ##
==========================================
+ Coverage   46.26%   46.29%   +0.03%     
==========================================
  Files         193      193              
  Lines       16093    16093              
==========================================
+ Hits         7445     7451       +6     
+ Misses       8259     8254       -5     
+ Partials      389      388       -1

@alshabib
Copy link
Author

Happy to fix the failing test except that I do not understand why it is failing :)

Let me know what I need to do and I'll do it.

Copy link
Contributor

@akalipetis akalipetis left a comment

Choose a reason for hiding this comment

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

I think that you just need to revert the changes in bindata.go and the validate step will be passed. At least, that was the issue for me.

@@ -87,7 +87,7 @@ func dataConfig_schema_v30Json() (*asset, error) {
return nil, err
}

info := bindataFileInfo{name: "data/config_schema_v3.0.json", size: 0, mode: os.FileMode(0), modTime: time.Unix(0, 0)}
Copy link
Contributor

Choose a reason for hiding this comment

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

You should probably revert the changes here, to fix the validate step.

@@ -107,7 +107,7 @@ func dataConfig_schema_v31Json() (*asset, error) {
return nil, err
}

info := bindataFileInfo{name: "data/config_schema_v3.1.json", size: 0, mode: os.FileMode(0), modTime: time.Unix(0, 0)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Here too

@@ -127,7 +127,7 @@ func dataConfig_schema_v32Json() (*asset, error) {
return nil, err
}

info := bindataFileInfo{name: "data/config_schema_v3.2.json", size: 0, mode: os.FileMode(0), modTime: time.Unix(0, 0)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Here too

@@ -147,12 +147,12 @@ func dataConfig_schema_v33Json() (*asset, error) {
return nil, err
}

info := bindataFileInfo{name: "data/config_schema_v3.3.json", size: 0, mode: os.FileMode(0), modTime: time.Unix(0, 0)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Here too

@@ -167,7 +167,7 @@ func dataConfig_schema_v34Json() (*asset, error) {
return nil, err
}

info := bindataFileInfo{name: "data/config_schema_v3.4.json", size: 0, mode: os.FileMode(0), modTime: time.Unix(0, 0)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Here too

@alshabib
Copy link
Author

alshabib commented Jul 29, 2017 via email

@alshabib
Copy link
Author

Now it's a code coverage test that fails. Not sure what I need to do, any tips welcome.

@akalipetis
Copy link
Contributor

Not sure about the actual rule, but seems like you need at least 50% of your new changes covered. I would advise on adding tests, in the same places I have for my own patch 😄 You can follow the same logic.

@alshabib
Copy link
Author

Seems like there was a partial outage of some infrastructure used by circle ci. I'll wait for someone to kick of another verification when the infrastructure is back up.

Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

Thanks! this is looking good, just need to fix the test failure

updateConfig = convertUpdateConfig(&composetypes.UpdateConfig{
Parallelism: &parallel,
})
assert.Equal(t, updateConfig.Parallelism, parallel)
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be &parallel (the test is failing), also the args are inverted, expected should be the second arg, and actual the third:

assert.Equal(t, &parallel, updateConfig.Parallelism)

@alshabib
Copy link
Author

alshabib commented Jul 31, 2017

Just uploaded a patch, forgot to cast and foolishly thought that I had run tests locally before pushing...

not sure why the validate step failed.

@docker docker deleted a comment from GordonTheTurtle Aug 1, 2017
Copy link
Collaborator

@vdemeester vdemeester 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
Member

#360 was merged; can you rebase to get rid of that commit?

dnephin and others added 6 commits August 1, 2017 14:17
Split out a swarmCAOptions struct for options that are shared between
the ca and update commands.

Change the 'no trust root' message to an error.

Add some unit tests.

Signed-off-by: Daniel Nephin <[email protected]>
This package is used by Notary. Add a comment to
the vendor.conf file to explain what it contains
and what it's used for.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: Victor Vieux <[email protected]>
albers and others added 24 commits August 1, 2017 14:17
…ity`

This adds bash completion for moby/moby#32874.

Signed-off-by: Harald Albers <[email protected]>
Cleanup a test

Signed-off-by: Daniel Nephin <[email protected]>
Signed-off-by: Daniel Nephin <[email protected]>
The "until" filter is supported by all object types, except for
volumes.

Before this patch, the "until" filter would attempted to be used for the volume
prune endpoint, resulting in an error being returned by the daemon, and
further prune endpoints (networks, images) to be skipped.

    $ docker system prune --filter until=24h --filter label=label.foo=bar

    WARNING! This will remove:
            - all stopped containers
            - all volumes not used by at least one container
            - all networks not used by at least one container
            - all dangling images
    Are you sure you want to continue? [y/N] y
    Error response from daemon: Invalid filter 'until'

    Calling POST /v1.30/containers/prune?filters=%7B%22label%22%3A%7B%22label.foo%3D%3Dbar%22%3Atrue%7D%2C%22until%22%3A%7B%2224h%22%3Atrue%7D%7D
    Calling POST /v1.30/volumes/prune?filters=%7B%22label%22%3A%7B%22label.foo%3D%3Dbar%22%3Atrue%7D%2C%22until%22%3A%7B%2224h%22%3Atrue%7D%7D
    Handler for POST /v1.30/volumes/prune returned error: Invalid filter 'until'
    Error response from daemon: Invalid filter 'until'

With this patch, an error is produced instead, preventing "partial" prune.

    $ docker system prune --filter until=24h --filter label=foo==bar --volumes
    ERROR: The "until" filter is not supported with "--volumes"

Note that `docker volume prune` does not have this problem, and produces an
error if the `until` filter is used;

    $ docker volume prune --filter until=24h

    WARNING! This will remove all volumes not used by at least one container.
    Are you sure you want to continue? [y/N] y
    Error response from daemon: Invalid filter 'until'

Signed-off-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
Fix docker cp dir with hard link, refer to moby/moby#3.

Signed-off-by: Shukui Yang <[email protected]>
Signed-off-by: Daniel Nephin <[email protected]>
Signed-off-by: Liping Xue <[email protected]>
Change to enable volume name can be customized.
Signed-off-by: Liping Xue <[email protected]>

Change to enable volume name can be customized.

Remove unused debug info.

Address comments from Daniel and solve the lint error.
Signed-off-by: Liping Xue <[email protected]>

Address Daniel's comments to print warning message when name of external volume is set in loader code.
Signed-off-by: Liping Xue <[email protected]>

Address Daniel's comments to return error when external volume is set in loader code.
Signed-off-by: Liping Xue <[email protected]>

Address Daniel's comments to return error when external volume is set in loader code.
Signed-off-by: Liping Xue <[email protected]>

Remove the case that specifying external volume name in full-example.yml.

More fix.

Add unit test.
Signed-off-by: Liping Xue <[email protected]>

Address comments from Daniel, move the schema change to v3.4.
Signed-off-by: Liping Xue <[email protected]>

Address comments from Sebastiaan. Signed-off-by: Liping Xue <[email protected]>

Address comments from Misty.
Signed-off-by: Liping Xue <[email protected]>
Since CLI was moved to a separate repo, these references are incorrect.
Fixed with the help of sed script, verified manually.

Signed-off-by: Kir Kolyshkin <[email protected]>
@alshabib
Copy link
Author

alshabib commented Aug 1, 2017

Apologies. Managed to rebase the wrong branch on my work. Recreated here -> #409

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.