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

Compose services #45231

Merged
merged 7 commits into from Mar 5, 2018
Merged

Compose services #45231

merged 7 commits into from Mar 5, 2018

Conversation

obeleh
Copy link
Contributor

@obeleh obeleh commented Jan 2, 2018

What does this PR do?

Ads functionality to manage docker-compose service definitions as proposed in #44911

What issues does this PR fix or reference?

#44911

New Behavior

the following salt calls:

  • dockercompose.service_create
  • dockercompose. service_upsert
  • dockercompose.service_remove
  • dockercompose.service_set_tag

Different in the behaviour with all other salt calls in this module is that this has to do a yaml.dumps because we're modifying content and not modifying strings. I've explicitly stated in the docstrings that this is the case:

This wil re-write your yaml file. Comments will be lost. Indentation is set to 2 spaces

Tests written?

No

Commits signed with GPG?

Yes

@rallytime rallytime requested a review from terminalmage Jan 2, 2018
@rallytime
Copy link
Contributor

@rallytime rallytime commented Jan 2, 2018

re-run lint

Copy link
Collaborator

@terminalmage terminalmage left a comment

Now that #45190 has been merged, we should wait for @rallytime to merge that forward into develop, and then this should be rebased so that we use salt.utils.yaml.dump() to do the yaml dumping.

@rallytime
Copy link
Contributor

@rallytime rallytime commented Jan 22, 2018

@obeleh The merge forward should be completed now. Can you address the comments above?

@obeleh
Copy link
Contributor Author

@obeleh obeleh commented Jan 22, 2018

Ok switched yaml implementation. I hoped however that I could change this:

services[service_name]['image'] = str('{0}:{1}'.format(image, tag))

into this:

services[service_name]['image'] ='{0}:{1}'.format(image, tag)

But I still get this error:

yaml.constructor.ConstructorError: could not determine a constructor for the tag 'tag:yaml.org,2002:python/unicode'
 in "/etc/docker-compose/docker-oversight.yml", line 32, column 12

@rallytime rallytime requested a review from terminalmage Feb 8, 2018
Copy link
Collaborator

@terminalmage terminalmage left a comment

I'll work on the yaml dumper, we shouldn't be adding that YAML tag in our custom dumper.

None, None)
try:
with salt.utils.files.fopen(file_path, 'r') as fl:
loaded = yaml.load(fl)
Copy link
Collaborator

@terminalmage terminalmage Feb 8, 2018

Choose a reason for hiding this comment

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

We should be using safe_load here.

None, None)
else:
try:
loaded_definition = yaml.load(definition)
Copy link
Collaborator

@terminalmage terminalmage Feb 8, 2018

Choose a reason for hiding this comment

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

We should be using safe_load here.

def __write_docker_compose(path, docker_compose):
def __load_docker_compose(path):
'''
Read the compose file and load its' contents
Copy link
Collaborator

@terminalmage terminalmage Feb 8, 2018

Choose a reason for hiding this comment

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

no apostrophe necessary after its

@terminalmage
Copy link
Collaborator

@terminalmage terminalmage commented Feb 8, 2018

Oh sorry, I marked this PR as approved but I had a few changes I wanted to see made before merging.

Copy link
Collaborator

@terminalmage terminalmage left a comment

Accidentally marked as approved. See my earlier comments for a few changes that should be made.

Copy link
Collaborator

@terminalmage terminalmage left a comment

It turns out that using a regular dump here uses PyYAML's default dumper. Using our custom safe dumper, that YAML tag should not be present in the dumped output.

:return:
'''
try:
dumped = yaml.dump(content, indent=2, default_flow_style=False)
Copy link
Collaborator

@terminalmage terminalmage Feb 8, 2018

Choose a reason for hiding this comment

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

Please change this to safe_dump.

'Service {0} did not contain the variable "image"'.format(service_name),
None, None)
image = services[service_name]['image'].split(':')[0]
services[service_name]['image'] = str('{0}:{1}'.format(image, tag))
Copy link
Collaborator

@terminalmage terminalmage Feb 8, 2018

Choose a reason for hiding this comment

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

If you use safe_dump above, you can replace this with '{0}:{1}'.format(image, tag).

@rallytime
Copy link
Contributor

@rallytime rallytime commented Feb 28, 2018

@obeleh Can you come back to this PR and address the comments above?

@obeleh
Copy link
Contributor Author

@obeleh obeleh commented Mar 1, 2018

Sorry for the delay. At the time you asked for the change I was busy with other things. I've tested the change you suggested and it works. Hope everything is fine now :)

@rallytime rallytime requested a review from terminalmage Mar 1, 2018
@rallytime
Copy link
Contributor

@rallytime rallytime commented Mar 1, 2018

re-run lint

@rallytime
Copy link
Contributor

@rallytime rallytime commented Mar 5, 2018

@rallytime rallytime merged commit 39442c6 into saltstack:develop Mar 5, 2018
4 of 10 checks passed
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.

None yet

3 participants