Skip to content

Conversation

@allencloud
Copy link
Collaborator

Signed-off-by: Allen Sun [email protected]

Ⅰ. Describe what this PR did

remove InstanceInfo from container env since this part could be added in the plugin.
And this block of code is for special use, not so general.

@Ace-Tang

Ⅱ. Does this pull request fix one issue?

none

Ⅲ. Describe how you did it

none

Ⅳ. Describe how to verify it

none

Ⅴ. Special notes for reviews

none

@codecov-io
Copy link

codecov-io commented Jul 16, 2018

Codecov Report

Merging #1738 into master will decrease coverage by 0.64%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1738      +/-   ##
==========================================
- Coverage   14.14%   13.49%   -0.65%     
==========================================
  Files         281      281              
  Lines       56755    58270    +1515     
==========================================
- Hits         8027     7864     -163     
- Misses      47805    49476    +1671     
- Partials      923      930       +7
Impacted Files Coverage Δ
daemon/mgr/container_envs.go 66.66% <33.33%> (+21.79%) ⬆️
daemon/mgr/spec_process.go 45.18% <0%> (-22.6%) ⬇️
cri/v1alpha2/cri_utils.go 16.77% <0%> (-18.99%) ⬇️
pkg/user/user.go 59.15% <0%> (-16.61%) ⬇️
daemon/mgr/container.go 36.1% <0%> (-16.37%) ⬇️
cri/v1alpha1/cri_utils.go 19.28% <0%> (-15.64%) ⬇️
daemon/mgr/container_types.go 68.29% <0%> (-13.11%) ⬇️
daemon/mgr/container_exec.go 61.26% <0%> (-9.58%) ⬇️
daemon/logger/jsonfile/utils.go 71.54% <0%> (-1.63%) ⬇️
cri/v1alpha1/server.go 0% <0%> (ø) ⬆️
... and 8 more

func updateContainerEnv(inputRawEnv []string, baseFs string) error {
var (
envPropertiesPath = path.Join(baseFs, containerInstanceInfo)
envShPath = path.Join(baseFs, pouchEnvFile)
Copy link
Contributor

Choose a reason for hiding this comment

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

Where do we create envShPath and envShDir, are they create in rich-mode with runc?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The file is located in the image's rootfs.
In addition, the file is in the base image.

Copy link
Contributor

Choose a reason for hiding this comment

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

do you mean we not create the file, but only judge the file

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes

return nil
}
if _, err := os.Stat(envShPath); err != nil {
return fmt.Errorf("failed to stat container's env file /etc/profile.d/pouchenv.sh: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

if envShPath is not exist, we should return nil, as you said, we just judge the file i exist.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Using

	if _, err := os.Stat(envShPath); err != nil {
		logrus.Warnf("failed to stat container's env file /etc/profile.d/pouchenv.sh: %v", err)
		return nil
	}

? @Ace-Tang

@Ace-Tang
Copy link
Contributor

LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Jul 17, 2018
@Ace-Tang Ace-Tang merged commit f29327d into AliyunContainerService:master Jul 17, 2018
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/M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants