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

fix update spec annotation in runtime spec to support comma #2780

Conversation

wangforthinker
Copy link
Contributor

Signed-off-by: allen.wang allen.wq@alipay.com

Ⅰ. Describe what this PR did

fix cli update to support comma in annotation update.

Ⅱ. Does this pull request fix one issue?

none.

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

add cases.

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

Signed-off-by: allen.wang <allen.wq@alipay.com>
@codecov
Copy link

codecov bot commented Mar 28, 2019

Codecov Report

Merging #2780 into master will increase coverage by 0.65%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2780      +/-   ##
=========================================
+ Coverage   68.64%   69.3%   +0.65%     
=========================================
  Files         277     277              
  Lines       17425   17425              
=========================================
+ Hits        11961   12076     +115     
+ Misses       4108    4021      -87     
+ Partials     1356    1328      -28
Flag Coverage Δ
#criv1alpha2_test 39.35% <ø> (-0.03%) ⬇️
#integration_test_0 36.56% <ø> (+0.05%) ⬆️
#integration_test_1 35.75% <ø> (+0.01%) ⬆️
#integration_test_2 36.46% <ø> (-0.03%) ⬇️
#integration_test_3 35.45% <ø> (?)
#node_e2e_test 35.23% <ø> (-0.1%) ⬇️
#unittest 28.53% <ø> (ø) ⬆️
Impacted Files Coverage Δ
cri/ocicni/cni_manager.go 58.82% <0%> (-11.77%) ⬇️
pkg/streams/utils.go 82.14% <0%> (-7.15%) ⬇️
daemon/logger/jsonfile/utils.go 71.54% <0%> (-1.63%) ⬇️
cri/v1alpha2/cri.go 71.5% <0%> (-0.65%) ⬇️
daemon/mgr/container_utils.go 81.81% <0%> (-0.57%) ⬇️
daemon/mgr/spec_linux.go 78.77% <0%> (ø) ⬆️
daemon/mgr/image.go 59.45% <0%> (+0.54%) ⬆️
apis/server/router.go 91.35% <0%> (+0.61%) ⬆️
daemon/mgr/container_stats.go 71.83% <0%> (+1.14%) ⬆️
ctrd/container.go 57.17% <0%> (+1.33%) ⬆️
... and 14 more

@@ -55,7 +55,7 @@ func (uc *UpdateCommand) addFlags() {
flagSet.StringSliceVarP(&uc.labels, "label", "l", nil, "Update labels for container")
flagSet.StringVar(&uc.restartPolicy, "restart", "", "Restart policy to apply when container exits")
flagSet.StringSliceVar(&uc.diskQuota, "disk-quota", nil, "Update disk quota for container(/=10g)")
flagSet.StringSliceVar(&uc.specAnnotation, "annotation", nil, "Update annotation for runtime spec")
flagSet.StringArrayVar(&uc.specAnnotation, "annotation", nil, "Update annotation for runtime spec")
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 add more detailed description of specAnnotation in swagger:

      SpecAnnotation:
        description: "annotations send to runtime spec."
        type: "object"
        additionalProperties:
          type: "string"

Actually, users can hardly understand the actual meaning of it. Can you add some example for it in swagger.yml? @wangforthinker

@allencloud allencloud changed the title fix update annotation to support comma fix update spec annotation in runtime spec to support comma May 2, 2019
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@cardyok cardyok requested a review from Novicei July 7, 2022 12:17
@Novicei Novicei merged commit 6eca67b into AliyunContainerService:master Jul 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants