Skip to content

Conversation

@rhatdan
Copy link
Member

@rhatdan rhatdan commented Nov 21, 2017

No description provided.

@TomSweeneyRedHat
Copy link
Member

Sign this please @rhatdan

cmd/kpod/spec.go Outdated
}
return libpod.ErrNotImplemented
/*
pidNsPath := fmt.Sprintf("/proc/%d/ns/pid", state.Pid)
Copy link
Member

Choose a reason for hiding this comment

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

Is this commented code needed or will it be added shortly? A TODO comment maybe if it's going to stay?

Copy link
Member Author

Choose a reason for hiding this comment

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

Should add a TODO.

@TomSweeneyRedHat
Copy link
Member

Any changes/additions to the tests needed?

@rhatdan
Copy link
Member Author

rhatdan commented Nov 21, 2017

Yes I will add tests.

@mheon
Copy link
Member

mheon commented Nov 21, 2017

@rhatdan We have support baked into libpod for sharing namespaces, though it's not turned on yet - see https://github.com/projectatomic/libpod/blob/master/libpod/options.go#L300-L307

@rhatdan rhatdan force-pushed the pidns branch 4 times, most recently from e78247d to c5be531 Compare November 21, 2017 19:38
@rhatdan
Copy link
Member Author

rhatdan commented Nov 21, 2017

@TomSweeneyRedHat @mheon @umohnani8 @baude PTAL Tests passing.

@TomSweeneyRedHat
Copy link
Member

LGTM

@rhatdan
Copy link
Member Author

rhatdan commented Nov 22, 2017

Removed the TODO and added handling of the state.PID call for --pid containers:UUID.

cmd/kpod/spec.go Outdated
@@ -17,6 +17,27 @@ import (
"golang.org/x/sys/unix"
)

func addPidNS(config *createConfig, g *generate.Generator) error {
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a TODO here to switch to the libpod support for sharing namespaces when that lands?

@mheon
Copy link
Member

mheon commented Nov 22, 2017

One nit, otherwise LGTM. Tests are failing though.

@rhatdan rhatdan force-pushed the pidns branch 2 times, most recently from 8fc94da to 6eaf390 Compare November 22, 2017 15:25
@umohnani8
Copy link
Member

LGTM

@rh-atomic-bot
Copy link
Collaborator

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

@rhatdan rhatdan force-pushed the pidns branch 2 times, most recently from 2618cae to 7559a18 Compare November 22, 2017 15:56
Signed-off-by: Daniel J Walsh <[email protected]>
@rhatdan
Copy link
Member Author

rhatdan commented Nov 22, 2017

@rh-atomic-bot r=umohnani8

@rh-atomic-bot
Copy link
Collaborator

📌 Commit 38a49d2 has been approved by umohnani8

@rh-atomic-bot
Copy link
Collaborator

⌛ Testing commit 38a49d2 with merge bd4e106...

@rh-atomic-bot
Copy link
Collaborator

☀️ Test successful - status-papr
Approved by: umohnani8
Pushing bd4e106 to master...

baude pushed a commit to baude/podman that referenced this pull request Aug 31, 2019
Release: clean the builddir when building
@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 28, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 28, 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.

5 participants