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

all: convert package level vars to consts where possible #10329

Merged
merged 1 commit into from Feb 25, 2020
Merged

all: convert package level vars to consts where possible #10329

merged 1 commit into from Feb 25, 2020

Conversation

tklauser
Copy link
Member

@tklauser tklauser commented Feb 25, 2020 •
edited by tgraf

Also unexport them if they're not used outside the package.

Also remove unused global var RetryInterval, it's unused since commit
7ccf877 ("kvstore: New kvstore abstraction API").

This change is Reviewable

@tklauser tklauser added kind/cleanup This includes no functional changes. release-note/misc This PR makes changes that have no direct user impact. labels Feb 25, 2020
@tklauser tklauser requested a review from a team February 25, 2020 10:34
@tklauser tklauser requested review from a team as code owners February 25, 2020 10:34
@tklauser tklauser requested a review from a team February 25, 2020 10:34
@tklauser tklauser requested review from a team as code owners February 25, 2020 10:34
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 Feb 25, 2020
houndci-bot
houndci-bot reviewed Feb 25, 2020
View reviewed changes
operator/k8s_pod_controller.go
@@ -27,12 +27,12 @@ import (
)

const (
minimalPodRestartInterval = 5 * time.Minute
minimalPodRestartInterval = 5 * time.Minute
unmanagedKubeDnsMinimalAge = 30 * time.Second
Copy link

Choose a reason for hiding this comment

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

const unmanagedKubeDnsMinimalAge should be unmanagedKubeDNSMinimalAge

houndci-bot
houndci-bot reviewed Feb 25, 2020
View reviewed changes
operator/k8s_pod_controller.go
@@ -27,12 +27,12 @@ import (
)

const (
minimalPodRestartInterval = 5 * time.Minute
minimalPodRestartInterval = 5 * time.Minute
unmanagedKubeDnsMinimalAge = 30 * time.Second
Copy link

Choose a reason for hiding this comment

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

const unmanagedKubeDnsMinimalAge should be unmanagedKubeDNSMinimalAge

houndci-bot
houndci-bot reviewed Feb 25, 2020
View reviewed changes
pkg/maps/eppolicymap/eppolicymap.go Outdated

const (
MapName = "cilium_ep_to_policy"
Copy link

Choose a reason for hiding this comment

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

exported const MapName should have comment (or a comment on this block) or be unexported

@coveralls
Copy link

coveralls commented Feb 25, 2020 •
edited

Coverage Status

Coverage decreased (-0.03%) to 45.475% when pulling 0ceff80 on pr/tklauser/pkg-var-to-const into 37dd01d on master.

aanm
aanm approved these changes Feb 25, 2020
View reviewed changes
Copy link
Member

@aanm aanm left a comment •
edited

Choose a reason for hiding this comment

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

LGTM, there are at least 1 var that is changed when we are linking which is [0] so we can't move that one to const
[0]

cilium/pkg/version/version.go

Line 38 in 5accb00

var Version string

Also, is there any lint to check for this or are you doing it sort of manually?

@tklauser
Copy link
Member Author

LGTM, there are at least 1 var that is changed when we are linking which is [0] so we can't move that one to const
[0]

cilium/pkg/version/version.go

Line 38 in 5accb00

var Version string

Yes, I noticed that one and intentionally didn't change it. There are also others like pkg/envoy.RequiredEnvoyVersionSHA and pkg/datapath/loader.DatapathSHA which have corresponding comments. There are also some instances where a default value needs to be tunable for tests.

Also, is there any lint to check for this or are you doing it sort of manually?

I've done this manually (mainly looking through git grep -A 10 "^var (")

@aanm
Copy link
Member

aanm commented Feb 25, 2020 •
edited by tklauser

test-me-please

https://jenkins.cilium.io/job/Cilium-PR-Ginkgo-Tests-Validated/17470/testReport/junit/(root)/Suite-runtime/RuntimePrivilegedUnitTests_Run_Tests/

pkg/maps/eppolicymap.MapName and pkg/maps/eppolicymap.innerMapName are modified by during unit tests, so they need to be writable.

@pchaigno
Copy link
Member

I've done this manually (mainly looking through git grep -A 10 "^var (")

Maybe this would help?

for f in $(find . -path ./vendor -prune -o -name *.go); do \
  c=$(grep -vP "^\s*var\s*\(\s*\)" $f | awk '/^\s*var\s*\(/{flag=1}/^\s*\)/{flag=0}flag' | grep "="); \
  if [ -n "$c" ]; then echo $f; echo "$c"; fi; \
done

That's still 1203 lines to go through though 😥

@tklauser
all: convert package level vars to consts where possible
0ceff80 
Also unexport them if they're not used outside the package.

Also remove unused global var RetryInterval, it's unused since commit
7ccf877 ("kvstore: New kvstore abstraction API").

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
@tklauser tklauser requested a review from a team February 25, 2020 15:51
@tklauser tklauser requested a review from a team as a code owner February 25, 2020 15:51
@tklauser
Copy link
Member Author

test-me-please

@tgraf tgraf merged commit 2368655 into master Feb 25, 2020
1.8.0 automation moved this from In progress to Merged Feb 25, 2020
@tgraf tgraf deleted the pr/tklauser/pkg-var-to-const branch February 25, 2020 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@houndci-bot houndci-bot houndci-bot left review comments

@tgraf tgraf tgraf approved these changes

@aanm aanm aanm approved these changes

Assignees
No one assigned
Labels
kind/cleanup This includes no functional changes. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.8.0
  
Merged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@tklauser @coveralls @aanm @pchaigno @tgraf @houndci-bot