Fix up handling of environment variables #21

rhatdan · 2017-11-06T15:29:49Z

The way docker works is if a user specifies a non -e Name=Value, IE
just a -e Name, then the environment variable Name from the clients
OS.ENV is used.

Also by default Docker containers run with the HOSTNAME environment set
to the HOSTNAME specified for the container.

Signed-off-by: Daniel J Walsh dwalsh@redhat.com

rh-atomic-bot · 2017-11-07T13:33:27Z

☔ The latest upstream changes (presumably a39ec62) made this pull request unmergeable. Please resolve the merge conflicts.

rh-atomic-bot · 2017-11-15T22:58:49Z

☔ The latest upstream changes (presumably #47) made this pull request unmergeable. Please resolve the merge conflicts.

rhatdan · 2017-11-15T23:00:23Z

@baude @mheon @umohnani8 PTAL

mheon · 2017-11-16T15:22:06Z

cmd/kpod/create_cli.go

@@ -26,25 +26,6 @@ func getAllLabels(labelFile, inputLabels []string) (map[string]string, error) {
 	return labels, nil
 }

-func getAllEnvironmentVariables(envFiles, envInput []string) ([]string, error) {


Curious as to why you're breaking this up. It doesn't seem necessary to completely refactor it out - can we just pass in a spec generator and not return anything?

I am not breaking anything up? I removed a non useful function, that was throwing errors on
--env foo, commands, If you want me to make a separate function for the spec file generation, I can do that.

Alright, fair enough. Would be difficult to test anyways, given that we'd only be modifying the spec generator.

@mheon These three lines replace the getAllEnvironmentVariables.

- env, err := getAllEnvironmentVariables(c.StringSlice("env-file"), c.StringSlice("env")) + env, err := readKVStrings(c.StringSlice("env-file"), c.StringSlice("env")) if err != nil { return &createConfig{}, errors.Wrapf(err, "unable to process environment variables") } + env = append(defaultEnvVariables, env...)

TomSweeneyRedHat · 2017-11-16T18:05:48Z

cmd/kpod/spec.go

@@ -124,6 +128,22 @@ func createConfigToOCISpec(config *createConfig) (*spec.Spec, error) {
 		g.AddTmpfsMount(spliti[0], options)
 	}

+	for _, i := range config.env {


Did you decide to not append the defaultEnvVariables if none were found? i.e. line 34 from the deleted section of create_cli.go above.

We are always setting the default environment above in the patch.

env = append(defaultEnvVariables, env...)

I don't think the check is necessary, given that appending an empty array is a no-op

Missed that, thx, I'll blame the cold meds.

TomSweeneyRedHat · 2017-11-16T18:13:47Z

test/kpod_run.bats

+
+@test "run environment test" {
+
+#    run bash -c "${KPOD_BINARY} ${KPOD_OPTIONS} run ${ALPINE} sh -c printenv | grep HOSTNAME"


Curious why this is commented out?

We don't set the hostname in our containers since we are not using networks yet. As soon as we have network support for kpod run, we can uncomment this. I will add a comment on this.

mheon · 2017-11-16T18:24:59Z

cmd/kpod/create.go

 	if err != nil {
 		return &createConfig{}, errors.Wrapf(err, "unable to process environment variables")
 	}
+	env = append(defaultEnvVariables, env...)


Nit: this seems a little backward - usually would do append(env, defaultEnvVariables...) to make it more clear. Doesn't actually effect the results just a style preference

I did it this way so that if path is defined in defaultEnvVariables and in --env path, then the second path will replace the first path.

If I reverse then the user would not be able to override the defaults.

mheon · 2017-11-16T18:25:53Z

One nit, otherwise LGTM. Though I am also curious why part of the environment tests is commented out.

rhatdan · 2017-11-16T18:38:56Z

I reworked the function to setupEnvironment in spec.go, added a comment about the HOSTNAME check being commented out.
Added a test to make sure PATH= is overriden defaultEnvironment.

TomSweeneyRedHat · 2017-11-16T18:53:37Z

@rhatdan looks like you're getting bit by some travis environment bogosity.

TomSweeneyRedHat

LGTM once Travis gets happy. I don't think it's issues are due to this change.

rhatdan · 2017-11-17T15:42:09Z

bot retest

rhatdan · 2017-11-17T15:46:02Z

bot retry please

rhatdan · 2017-11-19T12:05:41Z

@jlebon @cgwalters Is there a way to keep the test image around, so we can log in and take a look around.
We are getting strange behaviour on the Centos test AMI, we think there could be a mismatch or some issue with the runc being used on this box.

We can not seem to get this failure on the centos machine we run and the error does not happen in travis or in the Fedora test machine.

The error we are seeing happens when the runc RuntimeSpec version does not match the kpod RunTime Spec version, and yet the rpm values look correct.

jlebon · 2017-11-19T15:18:32Z

Not currently, though you should be able to reproduce the same environment by starting from the same image it uses. But maybe first, let's move away from centos/7/atomic/alpha? It's no longer updated and the image there is pretty old 🕸 by now. Try centos/7/atomic/smoketested ✨ instead!

The corresponding image can be downloaded here to test locally: https://ci.centos.org/artifacts/sig-atomic/centos-continuous/images-smoketested/cloud/latest/images/.

The way docker works is if a user specifies a non `-e Name=Value`, IE just a `-e Name`, then the environment variable Name from the clients OS.ENV is used. Also by default Docker containers run with the HOSTNAME environment set to the HOSTNAME specified for the container. Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

rhatdan · 2017-11-20T13:24:34Z

Ok trying with smoketested

rhatdan · 2017-11-20T16:21:22Z

@baude @mheon PTAL

baude · 2017-11-20T16:25:27Z

@rh-atomic-bot r+ da86799

rh-atomic-bot · 2017-11-20T16:25:33Z

⚡ Test exempted: merge already tested.

rhatdan force-pushed the generate branch 5 times, most recently from 03a39d5 to e62e915 Compare November 6, 2017 23:37

rhatdan force-pushed the generate branch 2 times, most recently from 6d8b817 to f32d70f Compare November 8, 2017 20:51

rhatdan changed the title ~~Fix up handling of environment variables~~ [WIP] Fix up handling of environment variables Nov 9, 2017

rhatdan force-pushed the generate branch 2 times, most recently from a8d798b to 89a39a1 Compare November 12, 2017 11:34

rhatdan mentioned this pull request Nov 13, 2017

Fix terminal attach #32

Closed

rhatdan force-pushed the generate branch from 89a39a1 to 5d58589 Compare November 15, 2017 22:59

rhatdan changed the title ~~[WIP] Fix up handling of environment variables~~ Fix up handling of environment variables Nov 15, 2017

mheon reviewed Nov 16, 2017

View reviewed changes

TomSweeneyRedHat reviewed Nov 16, 2017

View reviewed changes

mheon reviewed Nov 16, 2017

View reviewed changes

rhatdan force-pushed the generate branch from 5d58589 to beebc5b Compare November 16, 2017 18:36

rhatdan force-pushed the generate branch from beebc5b to 52c2a9f Compare November 16, 2017 18:56

TomSweeneyRedHat reviewed Nov 16, 2017

View reviewed changes

rhatdan force-pushed the generate branch from 52c2a9f to d8d3ea9 Compare November 17, 2017 12:53

rhatdan force-pushed the generate branch from d8d3ea9 to f569beb Compare November 17, 2017 15:47

rhatdan force-pushed the generate branch 13 times, most recently from d99a6f6 to bd56d9f Compare November 18, 2017 12:08

rhatdan force-pushed the generate branch from bd56d9f to da86799 Compare November 20, 2017 13:24

rh-atomic-bot closed this in 57599f0 Nov 20, 2017

tyler92 mentioned this pull request Jul 20, 2022

libpod: do not lock all containers on pod rm #14976

Merged

odockal mentioned this pull request Aug 16, 2023

Spinning up containers from flask compose sample with docker-compose does not work with podman endpoint #19645

Closed

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 28, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 28, 2023


		@test "run environment test" {

		# run bash -c "${KPOD_BINARY} ${KPOD_OPTIONS} run ${ALPINE} sh -c printenv \| grep HOSTNAME"

Fix up handling of environment variables #21

Fix up handling of environment variables #21

Uh oh!

Conversation

rhatdan commented Nov 6, 2017

Uh oh!

rh-atomic-bot commented Nov 7, 2017

Uh oh!

rh-atomic-bot commented Nov 15, 2017

Uh oh!

rhatdan commented Nov 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mheon commented Nov 16, 2017

Uh oh!

rhatdan commented Nov 16, 2017

Uh oh!

TomSweeneyRedHat commented Nov 16, 2017

Uh oh!

TomSweeneyRedHat left a comment

Choose a reason for hiding this comment

Uh oh!

rhatdan commented Nov 17, 2017

Uh oh!

rhatdan commented Nov 17, 2017

Uh oh!

rhatdan commented Nov 19, 2017

Uh oh!

jlebon commented Nov 19, 2017

Uh oh!

rhatdan commented Nov 20, 2017

Uh oh!

rhatdan commented Nov 20, 2017

Uh oh!

baude commented Nov 20, 2017

Uh oh!

rh-atomic-bot commented Nov 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants