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

Fix typos and minor mistakes #196

Merged
merged 1 commit into from
Feb 8, 2022
Merged

Fix typos and minor mistakes #196

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions constraint/.golangci.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ linters:
- govet
- ineffassign
- misspell
- nilerr
- revive # replacement for golint
- staticcheck
- structcheck
Expand Down
6 changes: 4 additions & 2 deletions constraint/cmd/rewrite-compatibility/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,9 @@ opa test -v rewrite/lib/ rewrite/validator/
meld policy-library/lib/ rewrite/lib/
meld policy-library/validator/ rewrite/validator/
`,
RunE: rootCmdFn,
RunE: func(_ *cobra.Command, _ []string) error {
return rootCmdFn()
},
}

var (
Expand Down Expand Up @@ -115,7 +117,7 @@ func compileSrcs(
return nil
}

func rootCmdFn(cmd *cobra.Command, args []string) error {
func rootCmdFn() error {
return compileSrcs(cts, libs, pkgPrefix, oldRoot, newRoot)
}

Expand Down
16 changes: 0 additions & 16 deletions constraint/pkg/apis/externaldata/v1alpha1/provider_types_test.go
View file
Open in desktop

This file was deleted.

30 changes: 9 additions & 21 deletions constraint/pkg/apis/externaldata/v1alpha1/v1alpha1_suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,43 +16,31 @@ limitations under the License.
package v1alpha1

import (
"log"
"os"
"path/filepath"
"testing"

"k8s.io/client-go/kubernetes/scheme"
"k8s.io/client-go/rest"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/envtest"
)

var (
cfg *rest.Config
c client.Client
)

func TestMain(m *testing.M) {
t := &envtest.Environment{
func TestNewClient(t *testing.T) {
te := &envtest.Environment{
CRDDirectoryPaths: []string{filepath.Join("..", "..", "..", "..", "deploy", "crds.yaml")},
}

err := SchemeBuilder.AddToScheme(scheme.Scheme)
if err != nil {
log.Fatal(err)
}

if cfg, err = t.Start(); err != nil {
log.Fatal(err)
t.Fatal(err)
}

if c, err = client.New(cfg, client.Options{Scheme: scheme.Scheme}); err != nil {
log.Fatal(err)
cfg, err := te.Start()
if err != nil {
t.Fatal(err)
}

code := m.Run()
if err := t.Stop(); err != nil {
log.Fatal(err)
_, err = client.New(cfg, client.Options{Scheme: scheme.Scheme})
if err != nil {
t.Fatal(err)
}
os.Exit(code)
}
0 constraint/pkg/apis/templates/group.go → constraint/pkg/apis/templates/doc.go
View file
Open in desktop
File renamed without changes.
2 changes: 1 addition & 1 deletion constraint/pkg/client/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,7 +341,7 @@ func (c *Client) RemoveTemplate(ctx context.Context, templ *templates.Constraint
return resp, err
}

if err := c.backend.driver.RemoveTemplate(ctx, templ); err != nil {
if err := c.backend.driver.RemoveTemplate(templ); err != nil {
return resp, err
}

Expand Down
16 changes: 8 additions & 8 deletions constraint/pkg/client/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ func TestClient_AddData(t *testing.T) {
}

if r == nil {
t.Fatal("got AddTemplate() == nil, want non-nil")
t.Fatal("got AddData() == nil, want non-nil")
}

if diff := cmp.Diff(tc.wantHandled, r.Handled, cmpopts.EquateEmpty()); diff != "" {
Expand Down Expand Up @@ -461,7 +461,7 @@ func TestClient_RemoveTemplate(t *testing.T) {

r, err := c.RemoveTemplate(context.Background(), tc.template)
if err != nil {
t.Errorf("err = %v; want nil", err)
t.Fatalf("err = %v; want nil", err)
}

if diff := cmp.Diff(tc.wantHandled, r.Handled, cmpopts.EquateEmpty()); diff != "" {
Expand All @@ -482,7 +482,7 @@ func TestClient_RemoveTemplate_ByNameOnly(t *testing.T) {
{
name: "Good Template",
handler: &handlertest.Handler{},
template: cts.New(),
template: cts.New(cts.OptTargets(cts.Target(handlertest.HandlerName, cts.ModuleDeny))),
wantHandled: map[string]bool{handlertest.HandlerName: true},
wantError: nil,
},
Expand Down Expand Up @@ -527,7 +527,7 @@ func TestClient_RemoveTemplate_ByNameOnly(t *testing.T) {

r, err := c.RemoveTemplate(context.Background(), sparseTemplate)
if err != nil {
t.Errorf("err = %v; want nil", err)
t.Fatalf("err = %v; want nil", err)
}

if diff := cmp.Diff(tc.wantHandled, r.Handled, cmpopts.EquateEmpty()); diff != "" {
Expand Down Expand Up @@ -677,15 +677,15 @@ func TestClient_GetTemplate_ByNameOnly(t *testing.T) {
}

func TestClient_RemoveTemplate_CascadingDelete(t *testing.T) {
handler := &handlertest.Handler{}
h := &handlertest.Handler{}

d := local.New()
b, err := client.NewBackend(client.Driver(d))
if err != nil {
t.Fatalf("Could not create backend: %s", err)
}

c, err := b.NewClient(client.Targets(handler))
c, err := b.NewClient(client.Targets(h))
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -966,8 +966,8 @@ func TestClient_RemoveConstraint(t *testing.T) {
t.Fatalf("Could not create backend: %s", err)
}

handler := &handlertest.Handler{}
c, err := b.NewClient(client.Targets(handler))
h := &handlertest.Handler{}
c, err := b.NewClient(client.Targets(h))
if err != nil {
t.Fatal(err)
}
Expand Down
2 changes: 1 addition & 1 deletion constraint/pkg/client/drivers/interface.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ type Driver interface {
// AddTemplate adds the template source code to OPA
AddTemplate(ct *templates.ConstraintTemplate) error
// RemoveTemplate removes the template source code from OPA
RemoveTemplate(ctx context.Context, ct *templates.ConstraintTemplate) error
RemoveTemplate(ct *templates.ConstraintTemplate) error
// AddConstraint inserts validated constraint into OPA
AddConstraint(ctx context.Context, constraint *unstructured.Unstructured) error
// RemoveConstraint removes a constraint from OPA
Expand Down
12 changes: 9 additions & 3 deletions constraint/pkg/client/drivers/local/local.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -585,11 +585,17 @@ func (d *Driver) AddTemplate(templ *templates.ConstraintTemplate) error {
}

// RemoveTemplate implements driver.Driver.
// TODO: Cascading deletes.
func (d *Driver) RemoveTemplate(ctx context.Context, templ *templates.ConstraintTemplate) error {
if err := validateTargets(templ); err != nil {
func (d *Driver) RemoveTemplate(templ *templates.ConstraintTemplate) error {
if len(templ.Spec.Targets) != 1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

we probably want to return an error here to avoid failing silently if we forget to update this code when multi-target templates become a thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we return an error here, it's possible to get into a bad state where a template can't be deleted.

  1. Add valid Template
  2. Update Template to have multiple targets and wait for reconcile error
  3. Delete Template

As-is, this code is probably broken if someone does a similar change. I'd rather investigate and resolve in a dedicated PR than add the test/logic necessary to deal with this case here. Thus, I'd rather not modify the code and possibly introduce a bug (or modify the behavior of an existing one) and feel it safer to leave as-is.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also - this will be made moot by compiler sharding since we'll fix this anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

Filed #197 in the compiler sharding milestone.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also added TODO.

// TODO: Properly handle multi-target deletes.
// This is likely trivially covered by compiler sharding since we'll
// control how data is stored per-Template.
// https://github.com/open-policy-agent/frameworks/issues/197
// The template has an unexpected number of targets at removal time, so do
// nothing.
return nil
}

targetHandler := templ.Spec.Targets[0].Target
kind := templ.Spec.CRD.Spec.Names.Kind
namePrefix := createTemplatePath(targetHandler, kind)
Expand Down
3 changes: 2 additions & 1 deletion constraint/pkg/client/drivers/local/local_unit_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,7 +509,8 @@ violation[{"msg": "msg"}] {
if len(dr.modules) == 0 {
t.Errorf("driver failed to add module")
}
gotErr = dr.RemoveTemplate(context.Background(), tmpl)

gotErr = dr.RemoveTemplate(tmpl)
if gotErr != nil {
t.Errorf("err = %v; want nil", gotErr)
}
Expand Down
2 changes: 1 addition & 1 deletion constraint/pkg/client/drivers/remote/remote.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ func (d *driver) AddTemplate(ct *templates.ConstraintTemplate) error {
}

// RemoveTemplate implements driver.Driver.
func (d *driver) RemoveTemplate(ctx context.Context, ct *templates.ConstraintTemplate) error {
func (d *driver) RemoveTemplate(ct *templates.ConstraintTemplate) error {
panic("not implemented")
}

Expand Down
3 changes: 2 additions & 1 deletion constraint/pkg/externaldata/request.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ type Request struct {
Keys []string `json:"keys,omitempty"`
}

// NewRequest creates a new request for the external data provider.
// NewProviderRequest creates a new request for the external data provider.
func NewProviderRequest(keys []string) *ProviderRequest {
return &ProviderRequest{
APIVersion: "externaldata.gatekeeper.sh/v1alpha1",
Expand All @@ -35,6 +35,7 @@ func NewProviderRequest(keys []string) *ProviderRequest {
}
}

// ProviderKind strings are special string constants for Providers.
// +kubebuilder:validation:Enum=ProviderRequestKind;ProviderResponseKind
type ProviderKind string

Expand Down
2 changes: 1 addition & 1 deletion constraint/pkg/externaldata/response.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ type Response struct {
SystemError string `json:"systemError,omitempty"`
}

// Items is the struct that contains the key, value or error from a provider response.
// Item is the struct that contains the key, value or error from a provider response.
type Item struct {
// Key is the request from the provider.
Key string `json:"key,omitempty"`
Expand Down
2 changes: 1 addition & 1 deletion constraint/pkg/schema/structural.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,5 +41,5 @@ func CRDSchema(sch *runtime.Scheme, version string) (*schema.Structural, error)
return structural, nil
}

return nil, fmt.Errorf("No CRD version '%q'", version)
return nil, fmt.Errorf("no CRD version %q", version)
}