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.

Sign up for GitHub

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

Jump to bottom

Add env to [engines] for engine to use #198

Merged
merged 1 commit into from Jun 30, 2020
Merged

Add env to [engines] for engine to use #198

merged 1 commit into from Jun 30, 2020

Conversation

QiWang19
Copy link
Collaborator

@QiWang19 QiWang19 commented Jun 26, 2020 •
edited

Add env for setting variables for engine to use.
close #31

Signed-off-by: Qi Wang qiwan@redhat.com

@QiWang19 QiWang19 mentioned this pull request Jun 26, 2020
Merged
rhatdan
rhatdan approved these changes Jun 29, 2020
View reviewed changes
docs/containers.conf.5.md Outdated
@@ -256,6 +256,9 @@ conmon_path=[
]
```

**container_engine_env**=[]
Copy link
Member

Choose a reason for hiding this comment

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

Should be just called env, but this should be in the "engine" section.
When used inside of the code it would be
Engine.Env rather then Containers.Env

docs/containers.conf.5.md Outdated
@@ -256,6 +256,9 @@ conmon_path=[
]
```

**container_engine_env**=[]
Additional environment variables to apply to the container engine. For example "http_proxy=internal.proxy.company.com"
Copy link
Member

Choose a reason for hiding this comment

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

How about something like:

"Environment variables to be used when running the container engine. Example....
Note these environment variables will not be used within the container. Set the env section under containers, if you want
to set environment variables for the container.

pkg/config/config.go Outdated
@@ -195,6 +195,9 @@ type EngineConfig struct {
// The first path pointing to a valid file will be used.
ConmonPath []string `toml:"conmon_path,omitempty"`

// Additional environment variables to apply to the container engine. For example "http_proxy=internal.proxy.company.com"
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as above.

pkg/config/config.go Outdated
@@ -195,6 +195,9 @@ type EngineConfig struct {
// The first path pointing to a valid file will be used.
ConmonPath []string `toml:"conmon_path,omitempty"`

// Additional environment variables to apply to the container engine. For example "http_proxy=internal.proxy.company.com"
ContainerEngineEnv []string `toml:"container_engine_env,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Just Env

pkg/config/testdata/containers_default.conf Outdated
@@ -127,6 +127,9 @@ conmon_path = [
"/usr/local/sbin/conmon"
]

# Additional environment variables to apply to the container engine. For example "http_proxy=internal.proxy.company.com"
container_engine_env = ["http_proxy=internal.proxy.company.com", "foo=bar"]
Copy link
Member

Choose a reason for hiding this comment

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

Just env

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed.

@QiWang19 QiWang19 force-pushed the engine-env branch 2 times, most recently from fa81d05 to c66006e Compare June 29, 2020 18:08
@QiWang19
Copy link
Collaborator Author

PTAL @rhatdan @TomSweeneyRedHat

@QiWang19 QiWang19 changed the title Add container_engine_env for engine to use Add env to [engines] for engine to use Jun 29, 2020
@QiWang19
Add container_engine_env for engine to use
d7c2446 
Add container_env_var for setting envariables for engin to use.

Signed-off-by: Qi Wang <qiwan@redhat.com>
@rhatdan
Copy link
Member

rhatdan commented Jun 30, 2020

LGTM
@TomSweeneyRedHat @ashley_cui @vrothberg @mheon PTAL

vrothberg
vrothberg reviewed Jun 30, 2020
View reviewed changes
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM, a very nice feature!

@vrothberg vrothberg merged commit 3aa13b7 into containers:master Jun 30, 2020
mheon
mheon reviewed Jun 30, 2020
View reviewed changes
pkg/config/config.go
@@ -207,6 +207,9 @@ type EngineConfig struct {
// memory.
EnablePortReservation bool `toml:"enable_port_reservation,omitempty"`

// Environment variables to be used when running the container engine. For example "http_proxy=internal.proxy.company.com"
Env []string `toml:"env,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

If it's not too late (and we haven't cut a release yet, I think) - why is this []string and not map[string]string?

Copy link
Member

Choose a reason for hiding this comment

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

Environments are slices in the stdlib and commonly used like that across the stack. pkg/env allows to conveniently translate between slices and maps. Also, slices are much easier to configure in config file than maps.

mheon
mheon reviewed Jun 30, 2020
View reviewed changes
pkg/config/config.go
@@ -207,6 +207,9 @@ type EngineConfig struct {
// memory.
EnablePortReservation bool `toml:"enable_port_reservation,omitempty"`

// Environment variables to be used when running the container engine. For example "http_proxy=internal.proxy.company.com"
Copy link
Member

Choose a reason for hiding this comment

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

Are these intended to be passed to Podman and Buildah? We need to make that more clear, not everyone uses our terminology about container engine vs runtime.

Copy link
Member

Choose a reason for hiding this comment

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

Good point! Could you open an issue for that? I guess we need to run over the docs/configs/etc. and look out for that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They will be passed to Podman, Buildah. Are you suggesting something like this ?

Suggested change
// Environment variables to be used when running the container engine. For example "http_proxy=internal.proxy.company.com"
// Environment variables to be used when running the container engine (e.g., Podman, Buildah). For example "http_proxy=internal.proxy.company.com"

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that would be good!

QiWang19 added a commit to QiWang19/common that referenced this pull request Jun 30, 2020
@QiWang19
Change engine env to map
39bc2df 
Use map[string]string for engine env instead of a slice of string.
Follow comments under containers#198 (comment)

Signed-off-by: Qi Wang <qiwan@redhat.com>
@QiWang19 QiWang19 mentioned this pull request Jun 30, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mheon mheon mheon left review comments

@vrothberg vrothberg vrothberg left review comments

@rhatdan rhatdan rhatdan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

possibility to set a proxy directly in podman instead of set the system wide environment variable
4 participants
@QiWang19 @rhatdan @mheon @vrothberg