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 container information to .containerenv #2821

Merged
merged 1 commit into from
Nov 25, 2020

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Nov 24, 2020

We have been asked to leak some container information
and image information into the container to be used
by certain tools. (Toolbox and others)

Signed-off-by: Daniel J Walsh [email protected]

What type of PR is this?

/kind api-change
/kind bug
/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake
/kind other

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?


@rhatdan
Copy link
Member Author

rhatdan commented Nov 24, 2020

@containers/podman-maintainers PTAL
@cgwalters @debarshiray @juanje PTAL
Begins to fix containers/podman#6192

Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

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

LGTM

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan, saschagrunert

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@juanje
Copy link

juanje commented Nov 24, 2020

LGTM

run_linux.go Outdated
id=%q
image=%q
imageid=%q
`, Version, b.Container, b.ContainerID, b.FromImage, b.FromImageID)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`, Version, b.Container, b.ContainerID, b.FromImage, b.FromImageID)
`, Version, b.Container, b.ContainerID, b.FromImage, b.FromImageID, isRootless)

name=%q
id=%q
image=%q
imageid=%q
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
imageid=%q
imageid=%q
rootless=%q

containerenvPath := filepath.Join(path, "/run/.containerenv")
if err = os.MkdirAll(filepath.Dir(containerenvPath), 0755); err != nil {
return err
}
emptyFile, err := os.Create(containerenvPath)
if err != nil {

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
isRootless := fmt.Sprintf("%v", unshare.IsRootless())

@TomSweeneyRedHat
Copy link
Member

I'd add the rootless info too, we'll probably want it at some point and it was part of the discussion in the issue.
Otherwise LGTM

We have been asked to leak some container information
and image information into the container to be used
by certain tools. (Toolbox and others)

Signed-off-by: Daniel J Walsh <[email protected]>
@TomSweeneyRedHat
Copy link
Member

LGTM

Copy link
Contributor

@QiWang19 QiWang19 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 rhatdan added the lgtm label Nov 24, 2020
@openshift-merge-robot openshift-merge-robot merged commit 587e617 into containers:master Nov 25, 2020
@debarshiray
Copy link
Member

Thanks a lot, @rhatdan !

rootless = 1
}
// Populate the .containerenv with container information
containerenv := fmt.Sprintf(`\
Copy link
Member

Choose a reason for hiding this comment

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

This looks weird. Why is the \ 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.

Makes the code easier to read, i figure.

Copy link
Member

Choose a reason for hiding this comment

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

That backslash makes it into the file, though, which is what makes it look weird.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok perhaps we should remove it then.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants