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

feature: upgrade go-swagger to 0.17.1 #2350

Merged

Conversation

zhuangqh
Copy link
Contributor

Ⅰ. Describe what this PR did

upgrade go-swagger to 0.17.1

brief changelog of go-swagger 0.17.1

  • stricter validation of swagger spec file
  • complete and correct type validation
  • x-omitempty extension
  • general bufix

Ⅱ. Does this pull request fix one issue?

ready to fix all of the issue which were in relation to api parameters validation.

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗?)

Ⅳ. Describe how to verify it

It is compatible with current code but enable to validate types.

Ⅴ. Special notes for reviews

@codecov
Copy link

codecov bot commented Oct 23, 2018

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2350      +/-   ##
==========================================
- Coverage   68.15%   66.95%   -1.21%     
==========================================
  Files         265      265              
  Lines       18211    18211              
==========================================
- Hits        12412    12193     -219     
- Misses       4370     4628     +258     
+ Partials     1429     1390      -39
Flag Coverage Δ
#criv1alpha1test 31.51% <ø> (-0.17%) ⬇️
#criv1alpha2test ?
#integrationtest 39.59% <ø> (+0.04%) ⬆️
#nodee2etest 33.13% <ø> (+0.21%) ⬆️
#unittest 25.52% <ø> (ø) ⬆️
Impacted Files Coverage Δ
storage/quota/quota.go 11.45% <0%> (-38.03%) ⬇️
cri/v1alpha2/server.go 42.4% <0%> (-36.81%) ⬇️
storage/quota/prjquota.go 15.43% <0%> (-12.09%) ⬇️
daemon/mgr/container_storage.go 51.53% <0%> (-8.28%) ⬇️
cri/v1alpha2/cri_wrapper.go 54.8% <0%> (-6.41%) ⬇️
daemon/mgr/snapshot.go 89.85% <0%> (-4.35%) ⬇️
apis/server/utils.go 71.15% <0%> (-3.85%) ⬇️
cri/stream/remotecommand/httpstream.go 43.52% <0%> (-3.11%) ⬇️
cri/v1alpha2/cri_utils.go 87.28% <0%> (-3%) ⬇️
daemon/logger/jsonfile/utils.go 71.54% <0%> (-1.63%) ⬇️
... and 11 more

- name: "body"
in: "body"
schema:
$ref: "#/definitions/CheckpointCreateOptions"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you do this kind of change in another pull request? Since then swagger.yml refactoring has nothing to do with the swagger upgrade. WDYT? @zhuangqh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you do this kind of change in another pull request? Since then swagger.yml refactoring has nothing to do with the swagger upgrade. WDYT? @zhuangqh

Actually, this part of code is wrong, which failed to pass the stricter validation. Futhermore, it makes this PR fail to pass the CI. So this part of change should be included in this PR although it looks confused...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Roger that.

@@ -939,19 +929,20 @@ paths:
$ref: "#/responses/404ErrorResponse"
tags: ["Container"]

/containers/{id}/checkpoints/{id}:
/containers/{id}/checkpoints/{checkpointId}:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

error: duplicated parameter id

@zhuangqh zhuangqh force-pushed the upgrade-swagger-to-0-17-1 branch from 9eb5153 to b631268 Compare October 24, 2018 04:14
CpuRealtimeRuntime, CpusetCpus, CpusetMems, DeviceCgroupRules, KernelMemory, MemoryReservation,
MemorySwap, MemorySwappiness, NanoCpus, OomKillDisable, PidsLimit, CpuCount, CpuPercent,
IOMaximumIOps, IOMaximumBandwidth, IntelRdtL3Cbm, ScheLatSwitch, MemoryWmarkRatio, MemoryExtra,
MemoryForceEmptyCtl]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These fields should be included in encoded json string. However, they are not required. x-omitempty extension provide the ability to solve this situation.

@@ -12,7 +12,7 @@ readonly cmd="nsenter"
nsenter::check_version() {
local has_installed version

has_installed="$(command -v nsenter1 || echo false)"
has_installed="$(command -v nsenter || echo false)"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

minor CI bug

@@ -125,7 +125,7 @@ func (suite *PouchRunMemorySuite) TestRunWithMemoryswappiness(c *check.C) {
"--memory-swappiness", "-1",
"--name", cname, busyboxImage, "top")
DelContainerForceMultyTime(c, cname)
res.Assert(c, icmd.Success)
c.Assert(res.ExitCode, check.Equals, 1)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This command would be fail because validation really works.

@zhuangqh zhuangqh requested a review from allencloud October 24, 2018 07:08
@zhuangqh zhuangqh changed the title [WIP] feature: upgrade go-swagger to 0.17.1 feature: upgrade go-swagger to 0.17.1 Oct 24, 2018
@allencloud
Copy link
Collaborator

It is so good to see we have make swagger play more important role. I will make this move on as soon as possible.

@allencloud
Copy link
Collaborator

allencloud commented Oct 28, 2018

I am afraid that you still need to update Dockerfile, since in it there is https://github.com/alibaba/pouch/blob/5ad887d876d930999e19fd2c2e54f37a18e5754b/Dockerfile#L36-L38 :

# install swagger 0.12.0
RUN wget --quiet -O /bin/swagger https://github.com/go-swagger/go-swagger/releases/download/0.12.0/swagger_linux_amd64 \
    && chmod +x /bin/swagger

Here we need to update the swagger version to 0.17.1 as well. @zhuangqh

@allencloud
Copy link
Collaborator

allencloud commented Oct 29, 2018

LGTM
we will merge this until the swagger in Dockerfile issue has been solved.

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Oct 29, 2018
@zhuangqh zhuangqh force-pushed the upgrade-swagger-to-0-17-1 branch from b631268 to a55bbd5 Compare October 29, 2018 12:49
correct several test case

Signed-off-by: zhuangqh <zhuangqhc@gmail.com>
@zhuangqh zhuangqh force-pushed the upgrade-swagger-to-0-17-1 branch from a55bbd5 to 9fdc4e4 Compare October 29, 2018 12:53
@zhuangqh
Copy link
Contributor Author

LGTM
we will merge this until the swagger in Dockerfile issue has been solved.

has updated @allencloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature LGTM one maintainer or community participant agrees to merge the pull reuqest. size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants