Skip to content

policy/api: Rework Rule.MarshalJSON() to ease maintainability#11651

Merged
aanm merged 1 commit intomasterfrom
pr/pchaigno/rework-rule-marshaljson
May 27, 2020
Merged

policy/api: Rework Rule.MarshalJSON() to ease maintainability#11651
aanm merged 1 commit intomasterfrom
pr/pchaigno/rework-rule-marshaljson

Conversation

@pchaigno
Copy link
Copy Markdown
Member

@pchaigno pchaigno commented May 22, 2020

Instead of listing all fields of Rule manually, use reflect.

@aanm What do you think of this approach?

@pchaigno pchaigno added sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. release-note/misc This PR makes changes that have no direct user impact. labels May 22, 2020
@pchaigno pchaigno requested a review from aanm May 22, 2020 14:15
@coveralls
Copy link
Copy Markdown

coveralls commented May 22, 2020 •
edited
Loading

Coverage Status

Coverage increased (+0.02%) to 36.911% when pulling 78dbf4f on pr/pchaigno/rework-rule-marshaljson into a12c2ee on master.

@pchaigno pchaigno marked this pull request as ready for review May 23, 2020 08:35
@pchaigno pchaigno requested a review from a team as a code owner May 23, 2020 08:35
aanm
aanm requested changes May 25, 2020
View reviewed changes
Comment thread pkg/policy/api/rule.go Outdated
@pchaigno pchaigno force-pushed the pr/pchaigno/rework-rule-marshaljson branch from d61a72f to a390a6f Compare May 26, 2020 08:37
@pchaigno pchaigno requested a review from aanm May 26, 2020 08:53
@pchaigno pchaigno force-pushed the pr/pchaigno/rework-rule-marshaljson branch from a390a6f to 621b5d7 Compare May 26, 2020 08:58
@pchaigno @aanm
policy/api: Rework Rule.MarshalJSON() to ease maintainability
78dbf4f 
Instead of listing all fields of Rule manually, use type embedding.

Co-authored-by: André Martins <andre@cilium.io>
Signed-off-by: Paul Chaignon <paul@cilium.io>
@pchaigno pchaigno force-pushed the pr/pchaigno/rework-rule-marshaljson branch from 621b5d7 to 78dbf4f Compare May 26, 2020 19:26
@pchaigno
Copy link
Copy Markdown
Member Author

pchaigno commented May 26, 2020

test-me-please

@pchaigno
Copy link
Copy Markdown
Member Author

pchaigno commented May 26, 2020

@aanm I've updated with your diff and added you as co-author to the commit. Please take a look.

aanm
aanm approved these changes May 26, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@aanm aanm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm impressed that it worked

@aanm
Copy link
Copy Markdown
Member

aanm commented May 26, 2020

test-me-please

@pchaigno
Copy link
Copy Markdown
Member Author

pchaigno commented May 27, 2020 •
edited
Loading

Runtime-4.9 failed heavily. Other builds are fine, so probably not related to this PR.

retest-runtime

@pchaigno
Copy link
Copy Markdown
Member Author

pchaigno commented May 27, 2020

K8s-1.11-Kernel-netnext hit known flake #11634, K8s-1.17-Kernel-4.19 #11235. Other required builds are passing and GKE has the usual failures. I'm marking as ready.

@pchaigno pchaigno added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 27, 2020
@aanm aanm merged commit caf1641 into master May 27, 2020
@aanm aanm deleted the pr/pchaigno/rework-rule-marshaljson branch May 27, 2020 13:26
@pchaigno pchaigno mentioned this pull request Jun 1, 2020
Closed
34 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aanm aanm aanm approved these changes

Assignees

No one assigned

Labels

ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@pchaigno @coveralls @aanm