-
Notifications
You must be signed in to change notification settings - Fork 109
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
Fix the method for converting Docker Compose to K8 Manifest. #623
Conversation
Yay, your first pull request! 👍 A contributor will be by to give feedback soon. In the meantime, please review the Layer5 Community Welcome Guide and sure to join the community Slack. |
@leecalcote can you please review it? |
@nebula-aac, help, help |
483f5b9
to
32bb666
Compare
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.
My limited knowledge of this says it's good to go.
Signed-off-by: Pratham <[email protected]>
32bb666
to
53a791e
Compare
Signed-off-by: winkletinkle <[email protected]>
Conflicts resolved |
👀, @leecalcote, I don't see a response here. I can help on my path to becoming a maintainer. I know there is active contribution requirement. I have time. |
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.
@shanukun, this is great. Thank you for working on this. Related to Kind: List
, will you please contrast your effort here in context of this meshery/meshery PR - meshery/meshery#13233? These are similar topics. Are we simpatico here and headed in the same direction? // @souvikinator
Assumed: It was the case with prior versions of kompose, but not with the one now in use in meshkit. Now kompose returns a Current: Expected (after this PR): The PR meshery/meshery#13233 is attempting to expand the capabilities of |
err = convert(ConvertOpt) | ||
if err != nil { | ||
return "", err | ||
ConvertOpt := client.ConvertOptions{ |
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.
You might want to add the WithKomposeAnnotations
option, as it was previously being passed.
However, I’m not sure if the annotations are being put to any use.
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.
It is true by default.
I’ve kept the configuration exactly the same. The manifests generated by the client or by a previous implementation shouldn't differ in any way.
@shanukun @leecalcote This looks good. To summarize: This is a meshkit issue that arises when formatting the converted manifest to align with the format supported by the Meshery server, due to changes in the Kompose manifest format. The PR addresses two main points:
|
@shanukun, does it work? 😉 This is an obvious "yes", but it would be helpful if you included results of your tests and example Compose files. |
Does this work end-to-end when Meshery Server uses this revised function? |
This is some quality work, @shanukun. Is there a corresponding PR in meshery/meshery that upgrades to the new version of the kompose package? |
@shanukun would you like to raise an issue calling for the support of Compose Profiles? |
Thanks for your contribution to Meshery! 🎉 |
Description
Kind: List
was discarded by kompose in kubernetes/kompose#1541. That's why Meshkit is unable to format the output and is returning an empty K8 manifest string.kompose (> 1.31.1)
can support Docker Compose 3.9, although not the version (v1.26.2) used inmeshery/meshery
.We can simplify the implementation of the Convert method by updating kompose and using the new client/interface (kubernetes/kompose#1593). We will need to bump the kompose version in meshery too.
Also, it seems there is no support for Profiles in meshery. Right now it is simply ignored. Meshery's API and UI will need to be updated to accommodate it.
This PR fixes:
meshery/meshery#12495
Part of meshery/meshery#10450
Notes for Reviewers
Removing the format function and returning the output without formatting can be a straightforward fix. (quick fix).
Signed commits