search

containers / build

another build tool for container images (archived, see https://github.com/rkt/rkt/issues/4024)
Apache License 2.0
342 stars 80 forks source link

isolator: fixed isolator, added test #240

Closed cgonyeo closed 8 years ago

cgonyeo commented 8 years ago

acbuild isolator add ... was broken, producing the error:

isolator add: json: error calling MarshalJSON for type *types.App: invalid isolator

This commit fixes this, and adds a test for isolator add to be able to catch this type of regression in the future.

Fixes https://github.com/appc/acbuild/issues/238.

monder commented 8 years ago

That certainly fixes the problem, but maybe we should change the spec a bit?

Current spec to my mind is kindof an anti-pattern.

For example there is a public field that shouldn't be public Every object that implements the Isolator interface have to carry value hacks to their implementation and always to no forget it. Maybe its time to refactor some of it before it gets out of control with all that Marshal/Unmarshal magic?

What I suggest is to change this line to be something like this: https://play.golang.org/p/7aOR3Bonc9 That way the Marshal/Unmarshal logic will be kept privately in a single file as it should have been.

If its possible I can work on the POC PR.

cgonyeo commented 8 years ago

I'd be in favor of applying a fix for this issue with the current spec, and then revising the spec to make this fix less of an anti-pattern. If you'd be willing to make a PR against the spec that would be awesome.

jonboulle commented 8 years ago

LGTM