feat: Sync annotation unmarshaling in gator by anlandu · Pull Request #2734 · open-policy-agent/gatekeeper

anlandu · 2023-05-01T23:22:03Z

What this PR does / why we need it:
Enables unmarshaling of the sync annotation into a struct
Design Doc

Special notes for your reviewer:

Signed-off-by: Anlan Du <adu47249@gmail.com>

codecov-commenter · 2023-05-02T03:08:56Z

Codecov Report

Patch coverage: 91.42% and project coverage change: +0.06 🎉

Comparison is base (a5ebec1) 53.46% compared to head (491cb84) 53.53%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2734      +/-   ##
==========================================
+ Coverage   53.46%   53.53%   +0.06%     
==========================================
  Files         127      128       +1     
  Lines       11373    11408      +35     
==========================================
+ Hits         6081     6107      +26     
- Misses       4817     4824       +7     
- Partials      475      477       +2

Flag	Coverage Δ
unittests	`53.53% <91.42%> (+0.06%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/syncutil/parser/syncannotationreader.go	`91.42% <91.42%> (ø)`

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

julianKatz · 2023-05-02T18:16:22Z

What this PR does / why we need it: Enables unmarshaling of the sync annotation into a struct Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):

Special notes for your reviewer:

Plz remove unneeded text and include a link to the doc. Or perhaps we need a meta-issue to link all the work to.

julianKatz · 2023-05-02T18:19:27Z

@@ -1,6 +1,7 @@
 package reader


Overall generally LGTM but needs unit tests.

Some things we should test:

converting a "compact representation" to an "expended representation". The pure function will make this test clearer.

converting actual text on a fake template into a full SyncRequirements

Some error scenarios where the annotation is malformed (someone's bound to mess up the formatting, particularly if the annotation picks up steam in the community)

Thanks! Added tests--I think adding tests for ExpandCompactRequirements would be pretty redundant but lmk what you think

Signed-off-by: Anlan Du <adu47249@gmail.com>

acpana

This LGTM for now, thanks for working on this!

I'd love to see this being used in gator to either generate SyncSets (probably going to happen later) or validate that a (constraint_template, constraint, config object) tuple satisfies the requirements. This could help iron out any kinks and provide immediate value to gator users.

maxsmythe

LGTM! Sorry for the slow review cycle

Signed-off-by: Anlan Du <adu47249@gmail.com>

sozercan

see suggestions, otherwise LGTM

Co-authored-by: Sertaç Özercan <852750+sozercan@users.noreply.github.com> Signed-off-by: Anlan Du <adu47249@gmail.com>

Signed-off-by: Anlan Du <adu47249@gmail.com> Co-authored-by: Sertaç Özercan <852750+sozercan@users.noreply.github.com> Signed-off-by: Hy3n4 <hy3nk4@gmail.com>

In open-policy-agent/gatekeeper#2734 (comment) the decision was made to change the sync data anotation casing from `requiresSyncData` to `requires-sync-data`. This better fits k8s standards. This PR updates gatekeeper-library templates with the new annotation key. Signed-off-by: juliankatz <juliankatz@google.com>

Logic changes

ec6a457

Signed-off-by: Anlan Du <adu47249@gmail.com>

anlandu force-pushed the sync-annotation-unmarshaling branch from f0fb385 to ec6a457 Compare May 1, 2023 23:23

acpana requested a review from julianKatz May 1, 2023 23:39

Periods

805d852

Signed-off-by: Anlan Du <adu47249@gmail.com>

julianKatz reviewed May 2, 2023

View reviewed changes

maxsmythe reviewed May 3, 2023

View reviewed changes

Comment thread pkg/gator/reader/read_constraints.go Outdated

anlandu added 2 commits May 3, 2023 17:58

Move trimming

b14fd97

Signed-off-by: Anlan Du <adu47249@gmail.com>

Address comments

6f20882

Signed-off-by: Anlan Du <adu47249@gmail.com>

anlandu force-pushed the sync-annotation-unmarshaling branch from 5bf981e to 6f20882 Compare May 4, 2023 00:58

Move to syncutils pkg, add tests

1968f4c

Signed-off-by: Anlan Du <adu47249@gmail.com>

anlandu force-pushed the sync-annotation-unmarshaling branch from 44f9bb7 to 1968f4c Compare May 10, 2023 03:27

Fix comments

0998847

Signed-off-by: Anlan Du <adu47249@gmail.com>

anlandu marked this pull request as ready for review May 10, 2023 03:48

acpana approved these changes May 11, 2023

View reviewed changes

Comment thread pkg/syncutil/syncannotationreader.go Outdated

maxsmythe approved these changes May 18, 2023

View reviewed changes

sozercan reviewed May 19, 2023

View reviewed changes

Comment thread pkg/syncutil/syncannotationreader.go Outdated

apeabody mentioned this pull request May 19, 2023

Migrate require-sync CI to future gatekeeper 3.13 requires-sync-data unmarshal function open-policy-agent/gatekeeper-library#345

Closed

anlandu force-pushed the sync-annotation-unmarshaling branch 2 times, most recently from 6d029ce to 0998847 Compare May 19, 2023 23:10

anlandu added 2 commits May 19, 2023 16:11

Address comments

76c94c3

Signed-off-by: Anlan Du <adu47249@gmail.com>

Merge branch 'master' into sync-annotation-unmarshaling

8e2b9f1

sozercan reviewed May 19, 2023

View reviewed changes

Comment thread pkg/syncutil/parser/syncannotationreader.go Outdated

sozercan reviewed May 19, 2023

View reviewed changes

Comment thread pkg/syncutil/parser/syncannotationreader_test.go Outdated

sozercan approved these changes May 19, 2023

View reviewed changes

anlandu and others added 2 commits May 19, 2023 17:47

Update pkg/syncutil/parser/syncannotationreader.go

5f96186

Co-authored-by: Sertaç Özercan <852750+sozercan@users.noreply.github.com> Signed-off-by: Anlan Du <adu47249@gmail.com>

Update pkg/syncutil/parser/syncannotationreader_test.go

491cb84

Co-authored-by: Sertaç Özercan <852750+sozercan@users.noreply.github.com> Signed-off-by: Anlan Du <adu47249@gmail.com>

sozercan merged commit 3801633 into open-policy-agent:master May 20, 2023

This was referenced Jun 8, 2023

fix: make sync data annotation camelCase #2821

Closed

syncannotationreader.go uses incorrect annotation key #2820

Closed

julianKatz mentioned this pull request Jun 8, 2023

feat: Change template annotation to use k8s-style casing open-policy-agent/gatekeeper-library#352

Closed

anlandu deleted the sync-annotation-unmarshaling branch June 10, 2023 00:14

apeabody mentioned this pull request Jun 12, 2023

feat: update template and CI to requires-sync-data open-policy-agent/gatekeeper-library#355

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Sync annotation unmarshaling in gator#2734

feat: Sync annotation unmarshaling in gator#2734
sozercan merged 10 commits into
open-policy-agent:masterfrom
anlandu:sync-annotation-unmarshaling

anlandu commented May 1, 2023 •

edited

Loading

Uh oh!

codecov-commenter commented May 2, 2023 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

julianKatz commented May 2, 2023

Uh oh!

julianKatz May 2, 2023

Uh oh!

anlandu May 10, 2023

Uh oh!

Uh oh!

acpana left a comment

Uh oh!

Uh oh!

maxsmythe left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sozercan left a comment •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Conversation

anlandu commented May 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented May 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

julianKatz commented May 2, 2023

Uh oh!

julianKatz May 2, 2023

Choose a reason for hiding this comment

Uh oh!

anlandu May 10, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

acpana left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maxsmythe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sozercan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

anlandu commented May 1, 2023 •

edited

Loading

codecov-commenter commented May 2, 2023 •

edited

Loading

sozercan left a comment •

edited

Loading