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

firecracker-experimental: Fix indentation issues #1140

Merged

Conversation

mcastelino
Copy link
Contributor

Description of changes: Due to indentation issues swagger was unable to generate code.
Fix the indentation issues with vsock fields.

The error was

 swagger generate client -f ./firecracker-experimental.yaml --model-package=client/models --client-package=client
2019/06/18 17:20:06 validating spec ./firecracker-experimental.yaml
The swagger spec at "./firecracker-experimental.yaml" is invalid against swagger specification 2.0. see errors :
- definitions.TokenBucket.Vsock in body is a forbidden property
- paths./network-interfaces/{iface_id}./vsocks/{id} in body is a forbidden property

Signed-off-by: Manohar Castelino manohar.r.castelino@intel.com

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@andreeaflorescu
Copy link
Member

andreeaflorescu commented Jun 20, 2019

@mcastelino what tool are you using for testing the swagger file? We also have some tests for yamls but apparently they're not doing what they're supposed to do. We should add the tool you are using to our integration test system to make sure we don't break it in the future again.

P.S. I did searched for a swagger generate but I could only find this one: https://swagger.io/tools/swagger-codegen/ which doesn't seem to be it.

@mcastelino
Copy link
Contributor Author

@mcastelino what tool are you using for testing the swagger file? We also have some tests for yamls but apparently they're not doing what they're supposed to do. We should add the tool you are using to our integration test system to make sure we don't break it in the future again.

@andreeaflorescu I use https://github.com/go-swagger/go-swagger but I am open to use the same tool that you use. This seems to match what the firecracker-go-sdk seems to use.

/cc @nmeyerhans

@andreeaflorescu
Copy link
Member

@mcastelino we should add that tool to our dev container so we can use it in our CI.
We can do two things depending on how urgent is the fix for you:

  • merge this PR without the test and do a patch release on top of 0.17.0 to fix the swagger
  • add the swagger generator tool to our dev container and also write the integration test before merging this PR.

I prefer the second option but it might take some more time. Is this issue a blocker for you?

@mcastelino
Copy link
Contributor Author

I prefer the second option but it might take some more time. Is this issue a blocker for you?

@andreeaflorescu the second approach is better. This is not a blocker for us. We will carry the modified file in our repo till the upstream file is fixed.

kata-containers/runtime#1813

@nmeyerhans
Copy link
Contributor

We use the quay.io goswagger containers for code generation in the firecracker-go-sdk. There have been a few regressions that we've caught and fixed in the past as well, and you'll find some references to that in the swagger.yaml file's history.

Adding a proper swagger client generation step to the firecracker CI system would be a great guard against future occurrences.

@andreeaflorescu
Copy link
Member

@mcastelino we tried to find a tool that can be easy to use in Python, but failed. I think for now it's better to merge the fix as is. Since Kata Containers CI runs on Firecracker PRs now, we should not have this problem anymore in the future after you switch to consuming the swagger definition from our repository.

@mcastelino
Copy link
Contributor Author

@mcastelino we tried to find a tool that can be easy to use in Python, but failed. I think for now it's better to merge the fix as is. Since Kata Containers CI runs on Firecracker PRs now, we should not have this problem anymore in the future after you switch to consuming the swagger definition from our repository.

Sure. I can add a job to the Kata CI that validates the experimental and production yamls.

Today I do one time auto generation and check in the generated files each time we see a firecracker release.

@andreeaflorescu
Copy link
Member

@mcastelino any update on this PR? To merge it we also need the entry in CHANGELOG.md under the Fixed section.

@mcastelino
Copy link
Contributor Author

@mcastelino any update on this PR? To merge it we also need the entry in CHANGELOG.md under the Fixed section.

Will update PR today.

@mcastelino
Copy link
Contributor Author

@mcastelino any update on this PR? To merge it we also need the entry in CHANGELOG.md under the Fixed section.

@andreeaflorescu under which section should I add the fixed entry. 0.17.0 has already been released. So should it be added at the top? CHANGELOG.md

@andreeaflorescu
Copy link
Member

@mcastelino we usually add an Unreleased section at the top of the Changelog. When we release a new Firecracker version, we replace Unreleased with the release number. Here is an example: 0efb4aa

Due to indentation issues swagger was unable to generate code.
Fix the indentation issues with vsock fields.

Signed-off-by: Manohar Castelino <manohar.r.castelino@intel.com>
@mcastelino mcastelino force-pushed the topic/fc_vsock_swagger branch from 3b0fd6e to e34e032 Compare July 10, 2019 00:52
@dianpopa dianpopa merged commit bb7738a into firecracker-microvm:master Jul 10, 2019
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.

5 participants