Configured golangci-lint #51

palczyn · 2021-09-06T15:03:24Z

Issue: KNTK-192

danielfurman · 2021-09-06T15:30:24Z

Please squash the commits

danielfurman · 2021-09-07T08:29:13Z

@Opelord Please rename the commit to e.g. Configure golangci-lint and adjust the code to comply

Also, take a look: https://chris.beams.io/posts/git-commit/

mmac-m3a

I hope those linters are worth those 101 mouse clicks :-).

mmac-m3a · 2021-09-07T23:52:31Z

apiv6/localhost_apiserver/main.go

 	flag.Parse()
-	return
+	return address, cloudexport, synthetics


It is interesting that the code worked before without any value being returned ;-).

It was a naked return: https://tour.golang.org/basics/7 - possible, but unreadable

mmac-m3a · 2021-09-08T00:00:56Z

Waiting for @Fenthick to approve before merging.

danielfurman

Partially reviewed.

Renaming package names leads to too big diff - it is too hard to review. Please revert package renames (and suppress linter).

.golangci.yml

apiv6/kentikapi/client_integration_test.go

apiv6/kentikapi/httputil/httputil_test.go

apiv6/examples/synthetics/mesh_test/metrics_matrix.go

danielfurman · 2021-09-09T11:21:14Z

apiv6/kentikapi/client.go

@@ -32,7 +34,7 @@ type Config struct {
 	AuthEmail        string
 	AuthToken        string
 	RetryCfg         RetryConfig
-	// LogPayloads enables logging of request and response payloads.
+	// LogPayloads enables logging of request and response api_payloads.


Suggested change

// LogPayloads enables logging of request and response api_payloads.

// LogPayloads enables logging of request and response payloads.

danielfurman · 2021-09-09T11:27:11Z

apiv6/localhost_apiserver/cloudexport/go/routers.go

@@ -29,7 +30,7 @@ type Route struct {
 	HandlerFunc http.HandlerFunc
 }

-// Routes are a collection of defined api endpoints
+// Routes are a collection of defined api api_endpoints


Suggested change

// Routes are a collection of defined api api_endpoints

// Routes are a collection of defined api endpoints

danielfurman · 2021-09-09T11:31:07Z

apiv6/localhost_apiserver/synthetics/go/routers.go

@@ -29,7 +30,7 @@ type Route struct {
 	HandlerFunc http.HandlerFunc
 }

-// Routes are a collection of defined api endpoints
+// Routes are a collection of defined api api_endpoints


Suggested change

// Routes are a collection of defined api api_endpoints

// Routes are a collection of defined api endpoints

danielfurman

Finished

danielfurman · 2021-09-09T13:05:39Z

kentikapi/internal/api_payloads/interface.go

-	SNMPID               *models.ID            `json:"snmp_id,string,omitempty" request:"post" response:"get,post,put"`
-	SNMPSpeed            IntAsString           `json:"snmp_speed,omitempty"` // caveat, GET returns snmp_speed as string but POST and PUT as int
+	SNMPID    *models.ID  `json:"snmp_id,string,omitempty" request:"post" response:"get,post,put"`
+	SNMPSpeed IntAsString `json:"snmp_speed,omitempty"` // caveat, GET returns snmp_speed as string


If the comment is too long, put it above the field.

For VRF and VRFID too.

danielfurman · 2021-09-09T13:25:54Z

kentikapi/internal/api_payloads/query.go

@@ -146,6 +147,7 @@ type queryArrayItemPayload struct {
 	IsOverlay   *bool        `json:"isOverlay,omitempty"`
 }

+//nolint:nilerr


@mateuszmidor Is this intended that the error returned from queryToPayload() function is discarded here? It seems risky to me, because we're silencing possible conversion errors here and the library sends different query payload than user intended.

Error should be passed up the call chain

@Opelord Could you fix that here? It should be straightforward to pass the error up the call stack.

Yes, I will.

danielfurman · 2021-09-09T13:26:24Z

kentikapi/internal/api_payloads/query.go

+	Dimension    []string             `json:"dimension,omitempty"`
+	FiltersObj   *filtersPayload      `json:"filters_obj,omitempty"`
+	SavedFilters []savedFilterPayload `json:"saved_filters,omitempty"`
+	MatrixBy     []string             `json:"matrixBy" request:"post"` // matrixBy is required in request even if empty.


Please put comment above the field

danielfurman · 2021-09-09T13:33:56Z

kentikapi/internal/api_payloads/saved_filter.go

@@ -113,10 +114,11 @@ func (p filtersPayload) ToFilters() (models.Filters, error) {
 	}, nil
 }

+//nolint:revive // FiltersToPayload could be unexported but filtersToPayload already exists


We could have filtersToPayload() and filtersPointerToPayload() methods. So this one here would be named filtersToPayload(), because it takes value.

danielfurman · 2021-09-09T13:37:43Z

kentikapi/internal/resources/alerting.go

@@ -49,7 +49,8 @@ func (a *AlertingAPI) GetActiveAlerts(ctx context.Context, params models.AlertsQ
 	return response.ToAlarms(), nil
 }

-func (a *AlertingAPI) GetAlertsHistory(ctx context.Context, params models.AlertsQueryParams) ([]models.HistoricalAlert, error) {
+func (a *AlertingAPI) GetAlertsHistory(ctx context.Context,
+	params models.AlertsQueryParams) ([]models.HistoricalAlert, error) {


Please put line break after last input argument, so that input arguments and function body are not in line.

danielfurman · 2021-09-09T13:51:35Z

kentikapi/internal/resources/tags.go

@@ -1,4 +1,5 @@
-package api_resources
+//nolint:dupl


Please do not use package-level nolint, if possible

Where was this fixed? I'm, not seeing it.

danielfurman · 2021-09-09T13:52:15Z

kentikapi/internal/resources/users.go

@@ -1,4 +1,5 @@
-package api_resources
+//nolint:dupl


Please do not use package-level nolint if possible. (Put above duplicating code)

danielfurman · 2021-09-09T14:02:40Z

kentikapi/models/device.go

+	DeviceSNMNPIP       *string
+	DeviceSNMPCommunity *string
+	MinimizeSNMP        *bool
+	DeviceBGPType       *DeviceBGPType // Note: for DeviceBGPType = DeviceBGPTypeDevice,


Ditto: put comment above

danielfurman · 2021-09-09T14:05:39Z

kentikapi/tags_integration_test.go

@@ -1,3 +1,4 @@
+//nolint:dupl


Please put it next to duplicated code

danielfurman · 2021-09-09T14:05:46Z

kentikapi/users_integration_test.go

@@ -1,3 +1,4 @@
+//nolint:dupl


Please put it next to duplicated code

danielfurman · 2021-09-10T09:50:40Z

.golangci.yml

@@ -23,6 +23,14 @@ issues:
        - gochecknoglobals
        - lll
        - gomnd
+    # tags.go and users.go are called by dupl in entirety. Nolints on package level are not recommended,


I wasn't aware that those files are duplicated as a whole. In that case whole file could be excluded. I checked in docs that there are no package-level excludes. So the simplest solution is to put nolint statement in a file (but maybe not above package statement so that it would not be a package docstring, if possible).

https://golangci-lint.run/usage/false-positives/

It seems //nolint directive is ignored by godoc.
I will revert the changes to the previous commit as there is also no other way to nolint an entire file in another place.

palczyn · 2021-09-13T08:53:53Z

@mmac-m3a Daniel approved the changes. Can you approve too, because your review got cancelled after the first push with corrections.

mmac-m3a

I cannot look at it anymore ... and definitely don't want to go though 80+ clicks again :-).
... but, please, answer my questions.

mmac-m3a · 2021-09-14T00:25:28Z

kentikapi/internal/api_payloads/saved_filter.go

@@ -76,7 +77,7 @@ func SavedFilterToPayload(sf models.SavedFilter) savedFilterPayload {
 		FilterLevel:       sf.FilterLevel,
 		CreatedDate:       &sf.CreatedDate,
 		UpdatedDate:       &sf.UpdatedDate,
-		Filters:           FiltersToPayload(sf.Filters),
+		Filters:           filtersToPayload(sf.Filters),


The logic behind those renamings escapes me. At one place we are changing from (ugly) camelCase to (little bit less ugly) CamelCase (e.g. here)... and here it is the other way around. Is this forced by the linters (aren't they fighting each other)? Is this a type-name vs. function-name thing?

In Golang the difference between CamelCase and camelCase is the difference between an exported and unexported field, struct or function. (example) It is genereally discouraged to have an exported function (a function that can be used by other packages) return an unexported struct.
Now in this case we had an exported function return an unexported type but the function wasn't used in other packages. So instead of exporting the return I "unexported" (if this is even a word) the function.
In case of RestClient we came to conclusion that because functions returning this struct are used in other packages than the return type should be exported too.
golang/lint#210

mmac-m3a · 2021-09-14T00:30:24Z

kentikapi/internal/resources/tags.go

@@ -1,4 +1,5 @@
-package api_resources
+//nolint:dupl


Where was this fixed? I'm, not seeing it.

mmac-m3a · 2021-09-14T00:30:48Z

kentikapi/internal/resources/users.go

@@ -1,4 +1,5 @@
-package api_resources
+//nolint:dupl


palczyn · 2021-09-14T09:47:02Z

As for your question here it was solved in this conversation. The problem was that I was supposed to avoid package level nolints. The logic being that it's better to nolint only the block of code that is called by the linter. However in these files the linter highlighted all of their content as a duplicate of the other. I tried to move the exception to the linters configuration file but it hurt clarity of what is actually happening so we decided to move it back. Hence probably the confusion that nothing changed.

palczyn force-pushed the linters2 branch from 7fb50b1 to 9c7e59b Compare September 6, 2021 15:31

Configure golangci-lint and adjust the code to comply

6ff00a8

palczyn force-pushed the linters2 branch from 9c7e59b to 6ff00a8 Compare September 7, 2021 08:47

mmac-m3a previously approved these changes Sep 8, 2021

View reviewed changes

danielfurman suggested changes Sep 8, 2021

View reviewed changes

Revert package renames and suppressed linters

9b226c5

palczyn dismissed mmac-m3a’s stale review via 9b226c5 September 9, 2021 08:36

danielfurman suggested changes Sep 9, 2021

View reviewed changes

palczyn force-pushed the linters2 branch from c936e46 to c910925 Compare September 9, 2021 12:07

Implemented first batch of suggestions from review

4d88ebf

palczyn force-pushed the linters2 branch from c910925 to 4d88ebf Compare September 9, 2021 12:10

danielfurman suggested changes Sep 9, 2021

View reviewed changes

palczyn added 2 commits September 9, 2021 16:50

Implemented second batch of suggestions from review

6f8129d

Implemented third batch of suggestions from review

87b6b72

danielfurman suggested changes Sep 10, 2021

View reviewed changes

Implemented third batch of suggestions from review

4fbe6ad

danielfurman approved these changes Sep 10, 2021

View reviewed changes

palczyn mentioned this pull request Sep 13, 2021

Added configurable request timeout to API v5 client #53

Closed

mmac-m3a approved these changes Sep 14, 2021

View reviewed changes

mmac-m3a merged commit df5c436 into kentik:master Sep 14, 2021

	// LogPayloads enables logging of request and response api_payloads.
	// LogPayloads enables logging of request and response payloads.

	// Routes are a collection of defined api api_endpoints
	// Routes are a collection of defined api endpoints

Configured golangci-lint #51

Configured golangci-lint #51

Conversation

palczyn commented Sep 6, 2021

danielfurman commented Sep 6, 2021

danielfurman commented Sep 7, 2021

mmac-m3a left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmac-m3a commented Sep 8, 2021

danielfurman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielfurman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

palczyn commented Sep 13, 2021

mmac-m3a left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

palczyn Sep 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

palczyn commented Sep 14, 2021

palczyn Sep 14, 2021 •

edited

Loading