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

Add .containerenv file #533

Closed
wants to merge 4 commits into from
Closed

Conversation

mheon
Copy link
Member

@mheon mheon commented Mar 22, 2018

This will allow programs to easily identify they are running in a container

There was some discussion about putting it in /run, but I opted for just / to match Docker convention. Then again, we did rename the file...

This will allow programs to easily identify they are running in a
container

Signed-off-by: Matthew Heon <[email protected]>
@mheon
Copy link
Member Author

mheon commented Mar 22, 2018

Fixed #529

@baude
Copy link
Member

baude commented Mar 22, 2018

matt this looks good. i wonder if this should be documented somewhere? Im not sure where frankly or I would say so ... but seems like something people might want to know.

@baude
Copy link
Member

baude commented Mar 22, 2018

bot, retest this please

@TomSweeneyRedHat
Copy link
Member

LGTM and happy green test buttons.

@mheon
Copy link
Member Author

mheon commented Mar 23, 2018

@baude We could put it in the manpages of create and run, I suppose? We don't really have more extensive docs than that outside of the tutorial.

@baude
Copy link
Member

baude commented Mar 23, 2018

Sounds as good of a place as any ...

@rhatdan
Copy link
Member

rhatdan commented Mar 23, 2018

Should we move this to /run?

@mheon
Copy link
Member Author

mheon commented Mar 23, 2018

Comments addressed - added a section on files we create to the podman run manpage, and moved the file to /run

@@ -21,6 +21,14 @@ If the IMAGE is not already loaded then **podman run** will pull the IMAGE, and
all image dependencies, from the repository in the same way running **podman
pull** IMAGE, before it starts the container from that image.

Several files will be automatically created within the container when it is run.
Copy link
Member

Choose a reason for hiding this comment

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

Aren't these files created at create/init time? Perhaps "... within the container and then updated based on run time parameters when it is run."

Although to be honest I'm struggling with the wording here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Init does happen within podman run, so technically this is correct if we are talking about the run command? I don't know if there's an easy way to describe this bit within the manpage. Maybe we need a separate podman-containers manpage to describe things like this, so we can detail where in the lifecycle things happen?

Copy link
Member

Choose a reason for hiding this comment

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

A lifecycle thing would be a great tutorial/blog post. Just so folks don't pick nits, what would you think about just dropping "when it is run" from the first sentence.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that's a good compromise

@@ -672,6 +672,17 @@ func (c *Container) makeBindMounts() error {
c.state.BindMounts["/etc/hostname"] = hostnamePath
}

// Make .containerenv
Copy link
Member

Choose a reason for hiding this comment

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

nit /run/.containerenv

Signed-off-by: Matthew Heon <[email protected]>
@mheon
Copy link
Member Author

mheon commented Mar 23, 2018

Added an environment variable to address a comment from @nalind in #529 - we now identify ourself as a container via the CONTAINER environment variable

Copy link
Member

@TomSweeneyRedHat TomSweeneyRedHat left a comment

Choose a reason for hiding this comment

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

LGTM

@rhatdan
Copy link
Member

rhatdan commented Mar 23, 2018

systemd has some fancy code that I can not find quickly that figures out if is / is the host / and this determines if it is in a different mount namespace so it is in a container. But I can not find the code after 30 seconds of searching.

@baude
Copy link
Member

baude commented Mar 23, 2018

bot, retest this please

@@ -898,6 +909,7 @@ func (c *Container) generateSpec() (*spec.Spec, error) {

g.SetHostname(c.Hostname())
g.AddProcessEnv("HOSTNAME", g.Spec().Hostname)
g.AddProcessEnv("CONTAINER", "libpod")
Copy link
Member

Choose a reason for hiding this comment

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

This should be lower-case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, fixed

Copy link
Member

Choose a reason for hiding this comment

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

I think this should be done by buildah and podman not by the library.
I would rather see the fact that buildah created me or podman rather then libpod.

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright, fair enough.

@mheon mheon force-pushed the containerized_file branch from c59b29c to a21b575 Compare March 23, 2018 14:50
@baude
Copy link
Member

baude commented Mar 23, 2018

@rh-atomic-bot r+

@rh-atomic-bot
Copy link
Collaborator

📌 Commit a21b575 has been approved by baude

@rh-atomic-bot
Copy link
Collaborator

⚡ Test exempted: merge already tested.

rh-atomic-bot pushed a commit that referenced this pull request Mar 23, 2018
Signed-off-by: Matthew Heon <[email protected]>

Closes: #533
Approved by: baude
rh-atomic-bot pushed a commit that referenced this pull request Mar 23, 2018
Signed-off-by: Matthew Heon <[email protected]>

Closes: #533
Approved by: baude
rh-atomic-bot pushed a commit that referenced this pull request Mar 23, 2018
Signed-off-by: Matthew Heon <[email protected]>

Closes: #533
Approved by: baude
@github-actions 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 27, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants