Skip to content

Conversation

niboo-ave
Copy link

Here is how I fixed the parsing.
Rather than append element and remove element in the list on which the loop is, I created a new list wherre I append the parsed_mount.
Then I replace it.

@GordonTheTurtle
Copy link

Please sign your commits following these rules:
https://github.com/docker/docker/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "issue1567" git@github.com:Niboo/docker-py.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842361558592
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

@niboo-ave niboo-ave changed the title Resolve Issue1567 Fix Issue1567 Apr 18, 2017
@niboo-ave
Copy link
Author

Sry this is my first Pull Request ever, not use to it. Is there anything else I can do for you ?

@niboo-ave
Copy link
Author

Any news ?

@shin-
Copy link
Contributor

shin- commented Apr 27, 2017

Hi Antoine,

Thank you for taking the time to write and submit a PR! Sorry for not responding earlier, I've been busy with other tasks lately. There's a few things I need before I can accept this PR:

  1. Please sign your commits
  2. Make sure the test_create_with_volume_mount passes. You can verify this locally by running make test in the project's root folder.
  3. As a bonus, if you were able to add a test checking that the behaviour that used to fail doesn't do so anymore, that would be really great. If that seems too daunting, let me know and I can add that myself.

@niboo-ave
Copy link
Author

niboo-ave commented May 2, 2017

Hey again, Thank for your answer.
Shoud I sign the 2 first commit too ?
I admit I don't realy got how those test work and why it failed. Can you help me with this.
To use the debugger I used these test in my own project:

def test_create_service_with_volume_mount(self):
     vol_name = 'banane'
     print vol_name
     container_spec = docker.types.ContainerSpec(
         'nib:oo', ['ls'],
         mounts=[
             docker.types.Mount(target='/test', source=vol_name)
         ]
     )
     # self.tmp_volumes.append(vol_name)
     task_tmpl = docker.types.TaskTemplate(container_spec)
     name = 'pomme'
     client = docker.APIClient(base_url='unix://var/run/docker.sock')
     svc_id = client.create_service(task_tmpl, name=name)
     svc_info = client.inspect_service(svc_id)
     print 'ContainerSpec' in svc_info['Spec']['TaskTemplate']
     cspec = svc_info['Spec']['TaskTemplate']['ContainerSpec']
     print 'Mounts' in cspec
     print len(cspec['Mounts']) == 1
     mount = cspec['Mounts'][0]
     print mount['Target'] == '/test'
     print mount['Source'] == vol_name
     print mount['Type'] == 'volume'

Every print is true (except the first one which is banane).
So the test should work right ?
In the original code you don't test if the mount element is already parsed into a dictionary but should I change my else into an elif isintance ?

Thanks for your answer and time.

@shin-
Copy link
Contributor

shin- commented May 5, 2017

Yes, every commit should be signed. Alternatively, you can squash them all into a single commit.

Changes look good now. Thanks!

Signed-off-by: Antoine Verlant <antoine@niboo.be>
@niboo-ave
Copy link
Author

Here it is.
Thanks for your support

@shin- shin- removed the dco/no label May 8, 2017
@shin- shin- added this to the 2.3.0 milestone May 8, 2017
Copy link
Contributor

@shin- shin- left a comment

Choose a reason for hiding this comment

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

Thank you!

@shin- shin- merged commit 933a303 into docker:master May 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants