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

Refactor ID mapping #240

Merged
merged 3 commits into from
Apr 27, 2018
Merged

Conversation

flx42
Copy link
Contributor

@flx42 flx42 commented Apr 25, 2018

Preliminary work for #239

Signed-off-by: Felix Abecassis <fabecassis@nvidia.com>
d829321 modified the idtools package and therefore default mapping
used by unpack, but raw-runtime-config still used the previous format.

Signed-off-by: Felix Abecassis <fabecassis@nvidia.com>
@flx42 flx42 mentioned this pull request Apr 26, 2018
@cyphar
Copy link
Member

cyphar commented Apr 26, 2018

Please run gofmt on the changed files -- that's what's causing the CI to fail. It would also be nice if we could put the new code inside utils_ux.go (and possibly making it follow the existing uxImage style where it is automatically applied based on the command type, thought I don't think we can abuse the categories for this...).

@flx42
Copy link
Contributor Author

flx42 commented Apr 26, 2018

Moved the command decorator to utils_ux.go and renamed it uxRemap.
Should we also move the ID parsing function to utils_ux.go though? This doesn't seem really right for me.

I don't think we can reuse the category mechanism for this since it can't be chained in its current form, the decorator approach can.

@cyphar
Copy link
Member

cyphar commented Apr 26, 2018

Should we also move the ID parsing function to utils_ux.go though? This doesn't seem really right for me.

No, but we could put it into utils.go. However, maybe we should use the same ctx.App.Metadata trick that we use for other parsed options. The downside is that you'd need to stuff in a MapOptions struct into it. This is something I could look into refactoring later if it makes it easier for you.

I don't think we can reuse the category mechanism for this since it can't be chained in its current form, the decorator approach can.

I will try to figure out a work-around for this after we merge it as well (since it's more of a maintenance thing than anything else).

Signed-off-by: Felix Abecassis <fabecassis@nvidia.com>
@flx42
Copy link
Contributor Author

flx42 commented Apr 27, 2018

It's now in utils.go.
For the rest, I agree that it might be worth it to postpone to a separate PR. It doesn't sound pretty to shove a struct inside a metadata string (do we really want to json encode/decode here?).

@cyphar
Copy link
Member

cyphar commented Apr 27, 2018

Sure, that's fine (though for reference the Metadata map is of interface{}, not string). Will review in a bit.

@cyphar
Copy link
Member

cyphar commented Apr 27, 2018

LGTM.

@cyphar cyphar merged commit db65f26 into opencontainers:master Apr 27, 2018
cyphar added a commit that referenced this pull request Apr 27, 2018
  cmd: refactor handling of ID mapping
  raw-runtime-config: fix the default rootless ID mapping
  doc: fix typo for umoci-raw-runtime-config(1)

LGTMs: @cyphar
Closes #240
@cyphar cyphar mentioned this pull request Apr 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants