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

tests: fix abuses of appc types.Isolator #2840

Merged
merged 1 commit into from Jun 28, 2016

Conversation

Projects
None yet
2 participants
@lucab
Member

lucab commented Jun 24, 2016

appc 0.8.5 performs stricter validation on types.Isolator instances,
and some rkt tests would fail due to misuses.
This commit switches to proper unmarshaling to construct those
instances.

Context at appc/spec#633

@jonboulle

This comment has been minimized.

Show comment
Hide comment
@jonboulle

jonboulle Jun 24, 2016

Contributor

we actually have constructors for cpu and memory for this exact reason - maybe we should add for caps?

Contributor

jonboulle commented Jun 24, 2016

we actually have constructors for cpu and memory for this exact reason - maybe we should add for caps?

@lucab

This comment has been minimized.

Show comment
Hide comment
@lucab

lucab Jun 24, 2016

Member

@jonboulle I'll switch to use those two here, so I can drop one more helper. Regarding you question, we could but we will end up with one constructor per-isolator (briefly discussed that with @alban yesterday, but we were not super happy about it). cpu/memory are special as they parse/transform values, but other isolators are straightforward to construct.

I think types.Isolator.UnmarshalJSON() is enough there (maybe it could gain a NewIsolator wrapper to be more obvious? But still no way to forbid direct construction).

EDIT: I'm actually mixing up things here, and we already have those constructors. They are just not very handy/useful in this table-context though. Amending anyway as one helper is superfluous now.

Member

lucab commented Jun 24, 2016

@jonboulle I'll switch to use those two here, so I can drop one more helper. Regarding you question, we could but we will end up with one constructor per-isolator (briefly discussed that with @alban yesterday, but we were not super happy about it). cpu/memory are special as they parse/transform values, but other isolators are straightforward to construct.

I think types.Isolator.UnmarshalJSON() is enough there (maybe it could gain a NewIsolator wrapper to be more obvious? But still no way to forbid direct construction).

EDIT: I'm actually mixing up things here, and we already have those constructors. They are just not very handy/useful in this table-context though. Amending anyway as one helper is superfluous now.

@lucab

This comment has been minimized.

Show comment
Hide comment
@lucab

lucab Jun 24, 2016

Member

Amended.

Member

lucab commented Jun 24, 2016

Amended.

@jonboulle

This comment has been minimized.

Show comment
Hide comment
@jonboulle

jonboulle Jun 27, 2016

Contributor

LGTM after one nit

Contributor

jonboulle commented Jun 27, 2016

LGTM after one nit

tests: fix abuses of appc types.Isolator
appc 0.8.5 perform stricter validation on types.Isolator instances,
and some rkt tests would fail due to library misuses.
This commit switches to proper unmarshaling to construct those
instances.
@lucab

This comment has been minimized.

Show comment
Hide comment
@lucab

lucab Jun 27, 2016

Member

Rebased.

Member

lucab commented Jun 27, 2016

Rebased.

@jonboulle jonboulle merged commit 235971d into rkt:master Jun 28, 2016

12 checks passed

Jenkins Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
debian-8-flavor-coreos (i-57661ce2) Success!
Details
debian-8-flavor-coreos (i-ce671d7b) Success!
Details
fedora-22-flavor-coreos (i-3c671d89) Success!
Details
fedora-22-flavor-coreos (i-c8671d7d) Success!
Details
fedora-23-flavor-coreos (i-2d641e98) Success!
Details
fedora-23-flavor-coreos (i-fe641e4b) Success!
Details
fedora-24-flavor-coreos (i-32671d87) Success!
Details
fedora-24-flavor-coreos (i-d4671d61) Success!
Details
semaphoreci The build passed on Semaphore.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment