-
Notifications
You must be signed in to change notification settings - Fork 143
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
Move generate code into a separate library #131
Conversation
@mrunalp , PTAL. |
@@ -9,9 +9,8 @@ import ( | |||
"strconv" | |||
"strings" | |||
|
|||
"github.com/Sirupsen/logrus" | |||
"github.com/opencontainers/ocitools/libgen" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a reasonable amount of “what should we call this?” discussion in #128, which resolved around seccomp-gen
. If we're aiming for separate generate/validate/(test?) commands, we probably want something like:
ocitools
|-- cmd
| `-- ocitools
| |-- main.go
| |-- generate.go
| |-- validate.go
| …
|-- generate
| |-- generate.go
| |-- utils.go, etc.
| `-- seccomp (#128)
| |-- consts.go
| …
|-- validate
| |-- validate.go
| `-- utils.go
…
Shifting the import paths around while we settle into whatever layout we choose isn't a big deal as long as we don't cut releases. But I expect that the libraries will start picking up external consumers shortly after they're born, and we'll make life easier on those downstream folks if we don't churn the layout too much ;).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wking , this new package structure looks very different from the current package structure.
ocitools
|-- cmd
| `-- runtimetest
| |-- main.go
| |-- rlimit_linux.go
| |-- mount
| |-- mountinfo_freebsd.go
| |-- mountinfo.go
| |-- mountinfo_linux.go
| |-- mountinfo_solaris.go
| |-- mountinfo_unsupported.go
| |-- mountinfo_windows.go
|-- main.go
|-- generate.go
|-- validate.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On Mon, Jul 11, 2016 at 07:09:30AM -0700, hmeng-19 wrote:
@@ -9,9 +9,8 @@ import (
"strconv"
"strings"
- "github.com/Sirupsen/logrus"
- "github.com/opencontainers/ocitools/libgen"
@wking , this new package structure looks very different from the
current package structure…
Yeah, but if we're going to restructure things, I think we want to
take a big step to get to where we (think we) want to be, instead of
taking a bunch of short steps. The big step may not be perfect, but
it's got a better chance at being right than a small step after which
we already plan on making further changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wking proposed layout scheme looks reasonable to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wking , @mrunalp . Sounds good. I will modify the code structure as @wking suggested.
@wking , in your proposal, there are two generate.go
- one is under cmd/ocitools
, another is under generate
. What is their difference? The same problem happens to validate.go
.
I think all the files under generate
and validate
should be libraries, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On Mon, Jul 11, 2016 at 11:36:11AM -0700, hmeng-19 wrote:
@wking , in your proposal, there are two
generate.go
- one is
undercmd/ocitools
, another is undergenerate
. What is their
difference? The same problem happens tovalidate.go
.
The cmd/ocitools stuff is just thin bindings between the library code
(under generate/, validate/, etc.) and urfave/cli.
I think all the files under
generate
andvalidate
should be
libraries, right?
Yup.
On Fri, Jul 08, 2016 at 08:02:51PM -0700, hmeng-19 wrote:
This is also going to create some tedious conflicts with #54 (now |
} | ||
|
||
// AddIDMappings add the mappings of uid and gid into spec. | ||
func AddIDMappings(spec *rspec.Spec, uidMaps, gidMaps []string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be a single:
func ParseIDMappings(spec *rspec.Spec, idType string, maps []string) error {…}
which the command implementation can call with something like (untested, and I don't do much Go ;):
idTypes := ["uid", "gid"]
for idType := range idTypes {
maps := context.StringSlice(idType + "mappings")
if err := libgen.AddIDMappings(spec, idType, maps) err != nil {
return err
}
}
The approach your copy/pasting was acceptable for an internal helper, but it seems like we should aim for something DRYer for a library method. On the other hand, I'm fine cleaning this stuff up in follow-up PRs, as long as we don't wait long enough for API-breaking to become a concern.
@mrunalp , @wking , the CI checks failed because this PR tries to introduce a new package
|
@mrunalp , @wking , the new changes I made include:
The new changes focus on the package structure, does not really polish the function API. |
On Mon, Jul 11, 2016 at 02:37:44PM -0700, hmeng-19 wrote:
Actually, I think that's failing because you haven't added |
@wking , you are right. My |
On Mon, Jul 11, 2016 at 02:55:12PM -0700, hmeng-19 wrote:
Ah, the ‘ocitools’ entry in .gitignore should probably be ‘/ocitools’ I'm not sure what the ‘oci’ entry in .gitignore is for. It's from |
@wking , I think the problem is the pattern Please refer to here https://git-scm.com/docs/gitignore . |
Through fed976f look fine to me if we want to shuffle things around But I don't see a need to split “restructure and reconsider the API” |
On Tue, Jul 12, 2016 at 07:13:44AM -0700, Grant Seltzer Richman wrote:
Whatever's easier, but you can rebase your existing #128 branch once |
Flags: generateFlags, | ||
Before: before, | ||
Action: func(context *cli.Context) error { | ||
spec := generate.GetDefaultTemplate() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be generate.New()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, a rename suggestion for GetDefaultTemplate
to New
commit history needs to be cleaned up or they should be squashed. |
While this is a step in the right direction, I think API like the following will be easier to use: type generator struct {
spec *rspec.Spec
}
// Start from default template
specgen := generator.New()
// Make modifications
specgen.SetRootPath("/path/to/rootfs")
specgen.AddCapabilities(...)
specgen.AddTmpfsMount(...)
specgen.SetSelinuxLabel(...)
...
...
// Write to disk
specgen.Save("/path/to/file")
// Get back the spec object
spec := specgen.GetSpec()
// Load a template.json
specgen := generator.NewFromTemplate("/path/to/file")
specgen.AddMount(src, dest, options..)
specgen.Save("/some/other/path")
// Load existing spec object and modify it to generate a new spec
specgen := generator.NewFromSpec(someSpecObject)
specgen.SetNoNewPriviliges(true)
specgen.Save("/yet/another/path") |
On Wed, Jul 13, 2016 at 02:53:54PM -0700, Mrunal Patel wrote:
I'd load this from an io.Reader for more flexibility. Wrapping that |
Please push your updates so the latest version could be reviewed. Sent from my iPhone
|
} | ||
|
||
// ClearLinuxUIDMapping clears g.spec.Linux.UIDMappings. | ||
func (g Generator) ClearLinuxUIDMapping() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ClearLinuxUIDmappings
0e6dfc0 LGTM |
|
||
func checkCap(c string) error { | ||
isValid := false | ||
cp := fmt.Sprintf("CAP_%s", c) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't required. The list you are checking against doesn't have CAP_
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The list I am checking against is capability.List()
, which includes CAP_CHOWN
, CAP_DAC_OVERRIDE
, and so on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ignore my comment. I got your point.
--seccomp-default doesn't get populated
|
Also trying to add a capability or dropping it fails |
2a0235e
to
9e31200
Compare
@mrunalp , fixed the problems you pointed out and also tested all the other options, PTAL. |
Can you re-order or squash the commits? |
@mrunalp , no problem. Are you done checking the latest changes? |
Signed-off-by: Haiyan Meng <hmeng@redhat.com>
@hmeng-19 Yeah, the changes look good and am ready to merge. |
@mrunalp , I sqashed the commits. |
LGTM |
Move generate code into a separate library,
libgen
.To avoid using
cli.Context
insidelibgen
, themodify
function responds to get the command options, and transfer them into the functions insidelibgen
.The PR tries to fix #130 .