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

refactor: encapsulate codes into function FormatStatus #542

Merged
merged 2 commits into from
Jan 11, 2018

Conversation

zeppp
Copy link
Contributor

@zeppp zeppp commented Jan 10, 2018

Signed-off-by: zeppp [email protected]

1.Describe what this PR did
encapsulate codes into function FormatStatus
2.Does this pull request fix one issue?

3.Describe how you did it

4.Describe how to verify it

5.Special notes for reviews

@CLAassistant
Copy link

CLAassistant commented Jan 10, 2018

CLA assistant check
All committers have signed the CLA.

@allencloud
Copy link
Collaborator

Thanks a lot for your work. 🌹
Please add a unit test for this function. @zeppp

var status string

if meta.State.Status == types.StatusRunning || meta.State.Status == types.StatusPaused {
start, _ := time.Parse(utils.TimeLayout, meta.State.StartedAt)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please do not ignore this error.

@codecov-io
Copy link

codecov-io commented Jan 10, 2018

Codecov Report

Merging #542 into master will increase coverage by 0.25%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #542      +/-   ##
==========================================
+ Coverage   14.24%   14.49%   +0.25%     
==========================================
  Files          64       64              
  Lines        3159     3173      +14     
==========================================
+ Hits          450      460      +10     
- Misses       2664     2666       +2     
- Partials       45       47       +2
Impacted Files Coverage Δ
daemon/mgr/container_types.go 18.18% <71.42%> (+18.18%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 44d8270...e32c190. Read the comment docs.

@zeppp
Copy link
Contributor Author

zeppp commented Jan 10, 2018

Should I add unit test in utils_test.go or somewhere? @allencloud

@allencloud
Copy link
Collaborator

In daemon/mgr/container_types_test.go, so you can add this file to the current directory.

@allencloud allencloud self-assigned this Jan 10, 2018
@zeppp zeppp force-pushed the refactor-FormatStatus branch from 50b9718 to 4498efa Compare January 11, 2018 03:09
@pouchrobot pouchrobot added size/L and removed size/M labels Jan 11, 2018
}
} else {
container.Status = string(m.State.Status)
status, err := m.FormatStatus()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move this part above above container := types.Container{ and still in the for loop, then we can add status when constructing the instance, like:

		container := types.Container{
			ID:         m.ID,
			Names:      []string{m.Name},
			Image:      m.Config.Image,
			Command:    strings.Join(m.Config.Cmd, " "),
			Created:    t.UnixNano(),
			Labels:     m.Config.Labels,
			HostConfig: m.HostConfig,
            Status: status,
		}

@zeppp zeppp force-pushed the refactor-FormatStatus branch from 4498efa to e32c190 Compare January 11, 2018 03:43
@allencloud
Copy link
Collaborator

LGTM, much better now.

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Jan 11, 2018
@allencloud allencloud merged commit 580accd into AliyunContainerService:master Jan 11, 2018
@zeppp zeppp deleted the refactor-FormatStatus branch January 11, 2018 05:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/refactor 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.

5 participants