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

Give driver a map of handler names to client.TargetHandler #185

Merged
merged 13 commits into from
Jan 25, 2022
Merged

Give driver a map of handler names to client.TargetHandler #185

merged 13 commits into from
Jan 25, 2022

Conversation

davis-haba
Copy link
Contributor

Signed-off-by: davis-haba davishaba@google.com

@davis-haba
Copy link
Contributor Author

@willbeason

willbeason
willbeason requested changes Jan 19, 2022
View reviewed changes
Copy link
Member

@willbeason willbeason left a comment

Choose a reason for hiding this comment

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

Two minor changes, otherwise looks good

constraint/pkg/client/drivers/local/args.go Show resolved Hide resolved
constraint/pkg/client/drivers/local/local.go Outdated Show resolved Hide resolved
@davis-haba davis-haba force-pushed the add-handlers-to-driver branch 3 times, most recently from 7965325 to fd19a47 Compare January 20, 2022 14:24
@codecov-commenter
Copy link

codecov-commenter commented Jan 20, 2022 •
edited

Codecov Report

Merging #185 (9b38a81) into master (5d06ded) will increase coverage by 0.19%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #185      +/-   ##
==========================================
+ Coverage   46.64%   46.84%   +0.19%     
==========================================
  Files          60       59       -1     
  Lines        2830     2818      -12     
==========================================
  Hits         1320     1320              
+ Misses       1266     1254      -12     
  Partials      244      244
Flag Coverage Δ
unittests 46.84% <ø> (+0.19%) ⬆️

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

Impacted Files Coverage Δ
...eworks/constraint/pkg/client/drivers/local/args.go 75.00% <0.00%> (-8.34%) ⬇️
...y-agent/frameworks/constraint/pkg/client/client.go 71.98% <0.00%> (ø)
.../frameworks/constraint/pkg/client/crds/validate.go 85.71% <0.00%> (ø)
...works/constraint/pkg/client/drivers/local/local.go 64.66% <0.00%> (ø)
...gent/frameworks/constraint/pkg/client/error_map.go

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d06ded...9b38a81. Read the comment docs.

@davis-haba
Give driver a map of handler names to client.TargetHandler
b949017 
Signed-off-by: davis-haba <davishaba@google.com>
@davis-haba
re-add error_map.go to client
a0d1944 
Signed-off-by: davis-haba <davishaba@google.com>
@davis-haba
cleanup - remove error_map and targethandler from client
dcf7841 
Signed-off-by: davis-haba <davishaba@google.com>
@davis-haba
organize imports in local.go
5b20fc7 
Signed-off-by: davis-haba <davishaba@google.com>
@davis-haba
move local.driver.errors to separate package
cc7e4e0 
Signed-off-by: davis-haba <davishaba@google.com>
@davis-haba
Move error_map.go to new errors package. Fix tests.
643520a 
Signed-off-by: davis-haba <davishaba@google.com>
@davis-haba
Merge branch 'master' of https://github.com/open-policy-agent/frameworks
c869488 
 into add-handlers-to-driver

Signed-off-by: davis-haba <davishaba@google.com>
@davis-haba
gofmt and goimports client package
b7bb131 
Signed-off-by: davis-haba <davishaba@google.com>
@davis-haba
name import for pkg/client/errors clienterrors in local/local_unit_te…
9b38a81 
…st.go

Signed-off-by: davis-haba <davishaba@google.com>
willbeason
willbeason approved these changes Jan 21, 2022
View reviewed changes
Copy link
Member

@willbeason willbeason left a comment

Choose a reason for hiding this comment

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

Just minor changes, otherwise LGTM

constraint/pkg/client/client.go Outdated Show resolved Hide resolved
constraint/pkg/client/drivers/local/local_unit_test.go Outdated Show resolved Hide resolved
constraint/pkg/client/crds/validate.go Outdated Show resolved Hide resolved
@davis-haba
call all imports of pkg/client/errors clienterrors for consistency
36ff009 
Signed-off-by: davis-haba <davishaba@google.com>
@davis-haba
no newline after clienterrors import
c74f80c 
Signed-off-by: davis-haba <davishaba@google.com>
maxsmythe
maxsmythe approved these changes Jan 21, 2022
View reviewed changes
Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

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

LGTM after lint error fixes

@willbeason
Copy link
Member

You should be able to fix the lint errors with golangci-lint run --fix

@davis-haba
sort imports in client.go
bf47424 
Signed-off-by: davis-haba <davishaba@google.com>
@davis-haba
sort inports in validate.go and client_test.go
930e2b6 
Signed-off-by: davis-haba <davishaba@google.com>
ritazh
ritazh approved these changes Jan 24, 2022
View reviewed changes
Copy link
Member

@ritazh ritazh left a comment

Choose a reason for hiding this comment

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

lgtm

@willbeason willbeason merged commit 52025b3 into open-policy-agent:master Jan 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ritazh ritazh ritazh approved these changes

@willbeason willbeason willbeason approved these changes

@maxsmythe maxsmythe maxsmythe approved these changes

@shomron shomron Awaiting requested review from shomron

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@davis-haba @codecov-commenter @willbeason @ritazh @maxsmythe