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

bugfix: rm omitempty in resource fields #1505

Merged

Conversation

allencloud
Copy link
Collaborator

@allencloud allencloud commented Jun 11, 2018

Signed-off-by: Allen Sun allensun.shl@alibaba-inc.com

Ⅰ. Describe what this PR did

In #1491, @Ace-Tang mentioned that we should not omit zero-value fields in api data transfer.

This pull request removes omitempty for fields in Resources.

But I think the key point is finding a way to generate a field which is not required but has no attribute of omitempty.

But this seems not supported in swagger

Ⅱ. Does this pull request fix one issue?

fix #1491

Ⅲ. Describe how you did it

Add required parameters in swagger.

Ⅳ. Describe how to verify it

none

Ⅴ. Special notes for reviews

@pouchrobot pouchrobot added kind/bug This is bug report for project size/XL labels Jun 11, 2018
@allencloud allencloud force-pushed the rm-omitempty-in-resources branch 4 times, most recently from 5fc2632 to 8f254e7 Compare June 11, 2018 08:58
@Ace-Tang
Copy link
Contributor

It will be better to find a way to generate no omitempty struct in container config.

@Ace-Tang
Copy link
Contributor

And please make CI passed, @allencloud .

@@ -270,10 +270,6 @@ func (s *Server) updateContainer(ctx context.Context, rw http.ResponseWriter, re
if err := json.NewDecoder(req.Body).Decode(config); err != nil {
return httputils.NewHTTPError(err, http.StatusBadRequest)
}
// validate request body
if err := config.Validate(strfmt.NewFormats()); err != nil {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed the request body validation in update, then I think it will pass the CI. @Ace-Tang @HusterWan

@codecov-io
Copy link

codecov-io commented Jun 12, 2018

Codecov Report

Merging #1505 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1505      +/-   ##
==========================================
- Coverage   41.59%   41.58%   -0.02%     
==========================================
  Files         267      267              
  Lines       17366    17364       -2     
==========================================
- Hits         7223     7220       -3     
- Misses       9251     9253       +2     
+ Partials      892      891       -1
Impacted Files Coverage Δ
apis/server/container_bridge.go 84.54% <ø> (-0.14%) ⬇️
ctrd/image.go 76.57% <0%> (-2.86%) ⬇️
daemon/logger/jsonfile/utils.go 71.54% <0%> (-1.63%) ⬇️
daemon/mgr/container_utils.go 56.7% <0%> (+1.73%) ⬆️
apis/server/utils.go 64.28% <0%> (+4.76%) ⬆️

@rudyfly
Copy link
Collaborator

rudyfly commented Jun 13, 2018

@allencloud @Ace-Tang You can see: go-swagger/go-swagger#1189, we use swagger version is 0.12.0, and I cherry-pick this pr to 0.12.0 version, and it use x-omitempty=false to disable omitempty.

@pouchrobot
Copy link
Collaborator

ping @allencloud
Conflict happens after merging a previous commit.
Please rebase the branch against master and push it back again. Thanks a lot.

@Ace-Tang
Copy link
Contributor

Hi @allencloud , Please resolve conflicts, and lgtm

@pouchrobot pouchrobot added LGTM one maintainer or community participant agrees to merge the pull reuqest. and removed LGTM one maintainer or community participant agrees to merge the pull reuqest. labels Jun 21, 2018
Signed-off-by: Allen Sun <allensun.shl@alibaba-inc.com>
Signed-off-by: Allen Sun <allensun.shl@alibaba-inc.com>
@allencloud allencloud force-pushed the rm-omitempty-in-resources branch from 2b8a144 to f3e9cc3 Compare June 21, 2018 09:38
@Ace-Tang Ace-Tang merged commit 662ef5c into AliyunContainerService:master Jun 22, 2018
@allencloud
Copy link
Collaborator Author

Actually I have not used the newly swagger @rudyfly sent me. Does it meet your demand?
Do I should revert this? @Ace-Tang

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug This is bug report for project size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: pouch inspect can not show zero value
5 participants