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

Fix Quantity serialization #417

Merged

Conversation

@davidxia
Copy link
Contributor

@davidxia davidxia commented Nov 3, 2018

Fix failing YamlTest.testLoadAllFile()

by adding a CustomRepresenter for Quantity to Yaml.

The serialized Deployment YAML's resource requests won't necessarily be in the
same format as the original. But this PR prevents the serialization from
returning empty maps which is an improvement.

In the future, another PR can be made which could guarantee the original YAML
is returned when serializing Deployment back to YAML.

@brendandburns
Copy link
Contributor

@brendandburns brendandburns commented Nov 4, 2018

This is failing formatting check:

You need to run:

mvn fmt:format
``
Copy link
Contributor

@brendandburns brendandburns left a comment

This looks good to me, but needs formatting.

davidxia added 2 commits Nov 3, 2018
to show how V1Containers are not deserialized correctly by `Yaml.dumpAll()`.
`YamlTest.testLoadAllFile()` fails.
by adding a `CustomRepresenter` for `Quantity` to `Yaml`.

The serialized Deployment YAML's resource requests won't necessarily be in the
same format as the original. But this PR prevents the serialization from
returning empty maps which is an improvement.

In the future, another PR can be made which could guarantee the original YAML
is returned when serializing Deployment back to YAML.
@davidxia davidxia force-pushed the davidxia:fix-quantity-serialization branch from 93f1427 to 12b147c Nov 4, 2018
@davidxia davidxia changed the title Fix quantity serialization Fix Quantity serialization Nov 4, 2018
@davidxia
Copy link
Contributor Author

@davidxia davidxia commented Nov 4, 2018

@brendanburns thanks. Fixed the formatting.

cpu: 500m
requests:
memory: 4Gi
cpu: '2'

This comment has been minimized.

@davidxia

davidxia Nov 4, 2018
Author Contributor

Not sure why the quotes are here. I guess it's because QuantityFormatter.toBase10String() returns a String?

@brendandburns
Copy link
Contributor

@brendandburns brendandburns commented Nov 5, 2018

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

@k8s-ci-robot k8s-ci-robot commented Nov 5, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brendandburns, davidxia

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@brendandburns
Copy link
Contributor

@brendandburns brendandburns commented Nov 5, 2018

THanks for the PR!

@k8s-ci-robot k8s-ci-robot merged commit 6e621b2 into kubernetes-client:master Nov 5, 2018
3 checks passed
3 checks passed
cla/linuxfoundation davidxia authorized
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
tide In merge pool.
Details
@davidxia davidxia deleted the davidxia:fix-quantity-serialization branch Nov 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants