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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from May 27, 2020
Merged

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

merged 1 commit into from May 27, 2020

Conversation

pchaigno
Copy link
Member

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
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 May 22, 2020
@coveralls
Copy link

coveralls commented May 22, 2020 •
edited

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
pkg/policy/api/rule.go Outdated Show resolved Hide resolved
@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
Member Author

test-me-please

@pchaigno
Copy link
Member Author

@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
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
Member

aanm commented May 26, 2020

test-me-please

@pchaigno
Copy link
Member Author

pchaigno commented May 27, 2020 •
edited

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

retest-runtime

@pchaigno
Copy link
Member Author

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
1.8.0 automation moved this from In progress to Merged 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
No open projects
1.8.0
  
Merged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@pchaigno @coveralls @aanm