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

rootless: enable linger if /run/user/UID not exists #3397

Merged
merged 1 commit into from
Jun 28, 2019

Conversation

giuseppe
Copy link
Member

at least on Fedora 30 it creates the /run/user/UID directory for the
user logged in via ssh.

This needs to be done very early so that every other check when we
create the default configuration file will point to the correct
location.

Signed-off-by: Giuseppe Scrivano [email protected]

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe

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

@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M labels Jun 21, 2019
@TomSweeneyRedHat
Copy link
Member

LGTM assuming happy tests.

@rhatdan
Copy link
Member

rhatdan commented Jun 24, 2019

/lgtm
/hold

@openshift-ci-robot openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Jun 24, 2019
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 24, 2019
@giuseppe giuseppe force-pushed the fix-reboot branch 2 times, most recently from 67ccb3d to 821514c Compare June 25, 2019 10:23
@giuseppe giuseppe force-pushed the fix-reboot branch 2 times, most recently from 327d873 to bf40ee3 Compare June 25, 2019 13:36
@giuseppe
Copy link
Member Author

tests are green

@rhatdan
Copy link
Member

rhatdan commented Jun 25, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 25, 2019

runtimeDir := os.Getenv("XDG_RUNTIME_DIR")

runtimeDirLinger, err := rootless.EnableLinger()
Copy link
Member

Choose a reason for hiding this comment

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

Should mention this in the comment

Copy link
Member Author

Choose a reason for hiding this comment

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

would you like a comment about what runtimeDirLinger is?

Copy link
Member

Choose a reason for hiding this comment

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

I'd like something in the function's header comment to indicate that SetXdgRuntimeDir() also calls EnableLinger() - so we know where the call is happening from the docs.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed and pushed

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 25, 2019
@giuseppe
Copy link
Member Author

@rhatdan there is still your hold.

@mheon is there still time for this PR to hit the release?

@mheon
Copy link
Member

mheon commented Jun 25, 2019

Too late, sorry - just merged the tag.

@mheon
Copy link
Member

mheon commented Jun 25, 2019

We're probably going to need a 1.4.4 with the runc fixes, though - might be able to sneak this in there

@giuseppe
Copy link
Member Author

@rhatdan anything left to do here or we can merge?

@rhatdan
Copy link
Member

rhatdan commented Jun 27, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 27, 2019
@giuseppe
Copy link
Member Author

it is still on hold

@rh-atomic-bot
Copy link
Collaborator

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

@rhatdan
Copy link
Member

rhatdan commented Jun 27, 2019

Oops @giuseppe looks like you need a rebase
/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 27, 2019
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 27, 2019
@rh-atomic-bot
Copy link
Collaborator

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

at least on Fedora 30 it creates the /run/user/UID directory for the
user logged in via ssh.

This needs to be done very early so that every other check when we
create the default configuration file will point to the correct
location.

Closes: containers#3410

Signed-off-by: Giuseppe Scrivano <[email protected]>
@giuseppe
Copy link
Member Author

rebased

@rhatdan
Copy link
Member

rhatdan commented Jun 28, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 28, 2019
@openshift-merge-robot openshift-merge-robot merged commit 5e42bf0 into containers:master Jun 28, 2019
@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 26, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. 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.

7 participants