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: remove update image and fix bugs when update env #1196

Merged
merged 1 commit into from
Apr 25, 2018

Conversation

HusterWan
Copy link
Contributor

Signed-off-by: Michael Wan zirenwan@gmail.com

Ⅰ. Describe what this PR did

  1. remove update image: if user want to change image of a container, he/she should use upgrade interface
  2. fix update env bugs

Ⅱ. Does this pull request fix one issue?

fixes #1188

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@pouchrobot pouchrobot added kind/bug This is bug report for project size/L labels Apr 24, 2018
@codecov-io
Copy link

codecov-io commented Apr 24, 2018

Codecov Report

Merging #1196 into master will increase coverage by 0.04%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1196      +/-   ##
==========================================
+ Coverage   15.57%   15.61%   +0.04%     
==========================================
  Files         178      178              
  Lines       10454    10427      -27     
==========================================
  Hits         1628     1628              
+ Misses       8706     8679      -27     
  Partials      120      120
Impacted Files Coverage Δ
cli/update.go 0% <ø> (ø) ⬆️
daemon/mgr/container.go 0% <0%> (ø) ⬆️

c.meta.Config.Image = ref
c.meta.Image = ref
if c.IsRunning() && len(config.Env) > 0 {
return fmt.Errorf("Only can update the container's Env when it stopped")
}

if len(config.Env) != 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we could remove this line of code, since the for loop below could handle the situation of config.Env has a length of 0.

}

command.PouchRun("rm", "-f", name).Assert(c, icmd.Success)
}
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 an integration test to verify updating env for a running container? @HusterWan

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok!

Signed-off-by: Michael Wan <zirenwan@gmail.com>
@allencloud
Copy link
Collaborator

LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Apr 25, 2018
@allencloud allencloud merged commit a8c1729 into AliyunContainerService:master Apr 25, 2018
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 LGTM one maintainer or community participant agrees to merge the pull reuqest. size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ bug ] improve update
4 participants