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.

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

Add linter #302

Merged
merged 9 commits into from Nov 25, 2019
Merged

Add linter #302

merged 9 commits into from Nov 25, 2019

Conversation

sozercan
Copy link
Member

KBv2 update resolved some of the lint issues on scaffolding
This PR adds golangci-lint to makefile and CI and fixes some of the lint issues

Closes #230 and #165

Signed-off-by: Sertaç Özercan <sozercan@gmail.com>
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.

This is awesome!

pkg/audit/manager.go Outdated Show resolved Hide resolved
pkg/controller/config/config_controller.go Outdated Show resolved Hide resolved
pkg/controller/config/config_controller_suite_test.go Outdated Show resolved Hide resolved
pkg/controller/config/config_controller_suite_test.go Outdated Show resolved Hide resolved
pkg/controller/constraint/constraint_controller.go Outdated Show resolved Hide resolved
pkg/webhook/certs.go Outdated Show resolved Hide resolved
pkg/webhook/certs.go Outdated Show resolved Hide resolved
pkg/webhook/policy.go Outdated Show resolved Hide resolved
@@ -39,8 +39,8 @@ var (
codecs = serializer.NewCodecFactory(runtimeScheme)
deserializer = codecs.UniversalDeserializer()
disableEnforcementActionValidation = flag.Bool("disable-enforcementaction-validation", false, "disable validation of the enforcementAction field of a constraint")
webhookName = flag.String("webhook-name", "validation.gatekeeper.sh", "DEPRECATED: set this on the manifest YAML if needed")
Copy link
Contributor

Choose a reason for hiding this comment

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

removing webhookname flag altogether is a breaking change, though there are other changes, so may be fine

Copy link
Member

Choose a reason for hiding this comment

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

We should make the webhook name customizable and this should be the flag that allows users to provide that.

Copy link
Member Author

@sozercan sozercan Nov 22, 2019

Choose a reason for hiding this comment

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

@maxsmythe if we keep it, we'll have to add // nolint:varcheck,deadcode,unused to ignore linting

Copy link
Contributor

Choose a reason for hiding this comment

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

@ritazh Webhook name (as in validation.gatekeeper.sh) is fully customizable via VWH config,

Copy link
Member

Choose a reason for hiding this comment

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

but I dont think users can deploy VWH separately right?

Copy link
Member

Choose a reason for hiding this comment

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

or did you mean they can modify this value in the deploy.yaml:

https://github.com/open-policy-agent/gatekeeper/blob/master/deploy/gatekeeper_kubebuilder_v2.yaml#L358

If so, then I guess we can just remove this flag.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ya, the next time we release, that kubebuilder_v2 yaml will be the order of the day. We should probably have a changeover plan.

pkg/controller/constraint/constraint_controller.go Outdated Show resolved Hide resolved
Signed-off-by: Sertaç Özercan <sozercan@gmail.com>

# Conflicts:
#	.travis.yml
pkg/audit/manager.go Outdated Show resolved Hide resolved
Signed-off-by: Sertaç Özercan <sozercan@gmail.com>
Signed-off-by: Sertaç Özercan <sozercan@gmail.com>
pkg/webhook/certs.go Outdated Show resolved Hide resolved
Signed-off-by: Sertaç Özercan <sozercan@gmail.com>
Signed-off-by: Sertaç Özercan <sozercan@gmail.com>
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

Signed-off-by: Sertaç Özercan <sozercan@gmail.com>
@maxsmythe
Copy link
Contributor

LGTM

@maxsmythe
Copy link
Contributor

Just noticed that watchVitals became public, we should avoid leaking the internals of Watch Manager if we can

Signed-off-by: Sertaç Özercan <sozercan@gmail.com>
pkg/watch/manager.go Outdated Show resolved Hide resolved
var gvks []schema.GroupVersionKind
for k := range r.intent {
for k2 := range r.intent[k] {
gvks = append(gvks, k2)
Copy link
Contributor

Choose a reason for hiding this comment

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

This has the potential of double-counting GVKs, as more than one registrar could be listing a watch a single kind.

Part of the cleanup I was going to do was moving the registrar merge logic into the recordKeeper. I suggest:

func (r *recordKeeper) Get() map[schema.GroupVersionKind]vitals {
	r.intentMux.RLock()
	defer r.intentMux.RUnlock()
	cpy := make(map[string]map[schema.GroupVersionKind]vitals)
	for k := range r.intent {
		cpy[k] = make(map[schema.GroupVersionKind]vitals)
		for k2, v := range r.intent[k] {
			cpy[k][k2] = v
		}
	}
	managedKinds := make(map[schema.GroupVersionKind]vitals)
	for _, registrar := range cpy {
		for gvk, v := range registrar {
			if mk, ok := managedKinds[gvk]; ok {
				merged, err := mk.merge(v)
				if err != nil {
					return nil, nil, nil, err
				}
				managedKinds[gvk] = merged
			} else {
				managedKinds[gvk] = v
			}
		}
	}
        return managedKinds
}

Then GetGVK becomes:

func (r *recordKeeper) GetGVK() []schema.GroupVersionKind {
	var gvks []schema.GroupVersionKind
	for gvk := range r.Get() {
	   gvks = append(gvks, gvk)
       }
       return gvks

Note that a lock is not needed because the Get() function uses a read lock and returns a copy of the data, so this function is thread-safe without it.

Signed-off-by: Sertaç Özercan <sozercan@gmail.com>
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

@maxsmythe maxsmythe merged commit 2607bbe into open-policy-agent:master Nov 25, 2019
@sozercan sozercan deleted the lint branch November 25, 2019 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update to kubebuilder v2
3 participants