-
Notifications
You must be signed in to change notification settings - Fork 485
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
Implement override and reset analog to docker-compose #830
base: main
Are you sure you want to change the base?
Conversation
90c1a27
to
c47dddb
Compare
…ocker.com/compose/compose-file/13-merge/) Signed-off-by: Sebastian Sellmeier <[email protected]>
return service | ||
|
||
try: | ||
tagObj.value = serivce |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a typo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No you need to still normalize the values of the Override-tag to be valid in the end (as they are the ones used) but pass the override-tag through to the recursive merge to do the actual override. I used try to determinate if there is a tagObj defiend and if so get the normalized values to the tagObj and return it otherwise return the normalized service-definition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also this isn't my most recent version anymore as I'm locally working an a version based on the latest https://github.com/maurerle/podman-compose but for now using docker-compose again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
serivce
seems to be non-existing variable though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Edit: You are all right - missed the typo but that's not an issue/the latest state and needs a rebase.
How likely are my chances to get this approach merged, if I resubmit it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, haven't looked into viability of the PR so far. Could you point me to the part of the compose spec that this implements? I didn't find with couple minutes of googling and better I don't make an error here :) Whoops, it's in PR description
Overall I think podman-compose should support all things possible in non-swarm compose that can be implemented on podman. We may argue about the approach of implementation though.
What I will definitely ask that's not part of the PR right now is unit and integration tests.
class OverrideTag(yaml.YAMLObject): | ||
yaml_dumper = yaml.Dumper | ||
yaml_loader = yaml.SafeLoader | ||
yaml_tag = u'!override' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I remember correctly, u
prefix is from python 2 times. Am I correct that it's not needed here?
for item in value: | ||
values.append(item.value) | ||
|
||
self.value = values |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.value = [item.value for item in value]
@@ -1629,7 +1709,7 @@ def _parse_compose_file(self): | |||
compose["services"] = resolved_services | |||
if not getattr(args, "no_normalize", None): | |||
compose = normalize_final(compose, self.dirname) | |||
self.merged_yaml = yaml.safe_dump(compose) | |||
self.merged_yaml = yaml.dump(compose) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this strictly needed?
return service | ||
|
||
if isinstance(service, OverrideTag): | ||
tagObj = service |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use tag_obj spelling.
if isinstance(service, OverrideTag): | ||
tagObj = service | ||
service = tagObj.value | ||
print(subdir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unneeded print.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in principle.
Please add unit and integration tests.
Just to let you all know - I'm still interested in this feature but I have to put the PR from my side on-hold so if someone needs it / want's to take over - feel free 🙏🏻 |
Just ran into these tags not working, which is a real issue for me. |
https://docs.docker.com/compose/compose-file/13-merge/