-
Notifications
You must be signed in to change notification settings - Fork 101
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
Chore: Updates and Cleanup #89
base: master
Are you sure you want to change the base?
Conversation
Supersedes #85 |
Tested with latest 7.5.0, works fine. |
@jonnenauha would you like me to adjust anything here? |
bump 🎉 |
@@ -1,5 +1,16 @@ | |||
module github.com/jonnenauha/prometheus_varnish_exporter | |||
|
|||
go 1.12 | |||
go 1.22 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would go 1.22 be required? I get the version bump if deps require it, but im not using packages that need new APIs from later go releases.
Prom seems to require go 1.21 so we should target that. https://github.com/prometheus/client_golang/blob/main/go.mod#L3
Despite what the go.mod says here, whatever go version you have will be used to build the binary. Its not like if you have go 1.23 installed and build it, it's not going to be be like you built it with go 1.12. go mod tells you the minimum version you can use to build the code. So if you are using features released in later go versions you can enforce it here.
But I agree bumping the version up to 1.21 is sensible. It can probably enable some compiler features versus 1.12.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more note, the travis ci go version update is ofc importatnt as it pushes the offiicial releases to github. So that is great, I was more talking about the go.mod changes here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's just me bumping everything to latest, you are correct, we can set this to 1.21.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EDIT: Actually, since 1.23 has been released this is no longer correct. I wouldn't set 1.21 here since it is no longer supported:
Each major Go release is supported until there are two newer major releases.
@@ -114,25 +114,25 @@ type group struct { | |||
|
|||
var ( | |||
groups = []group{ | |||
group{name: "backend", prefixes: []string{ | |||
{name: "backend", prefixes: []string{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what auto formatter are you using to remove the struct name here? i have never seen the official go fmt do this? I think this is an unnecessary change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the result of gofmt -s
, it "simplifies" code. Defining the struct type here is redundant.
@@ -229,7 +229,7 @@ func cleanBackendName(name string) string { | |||
if strings.HasPrefix(name, "reload_") { | |||
dot := strings.Index(name, ".") | |||
if dot != -1 { | |||
name = name[dot + 1:] | |||
name = name[dot+1:] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this certainly is not go fmt either right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's just standard go fmt
.
$ git status
On branch master
Your branch is up to date with 'origin/master'.
$ go fmt ./prometheus.go
$ git diff
diff --git a/prometheus.go b/prometheus.go
index c54f642..533051e 100644
--- a/prometheus.go
+++ b/prometheus.go
@@ -229,7 +229,7 @@ func cleanBackendName(name string) string {
if strings.HasPrefix(name, "reload_") {
dot := strings.Index(name, ".")
if dot != -1 {
- name = name[dot + 1:]
+ name = name[dot+1:]
}
}
@@ -1,8 +1,8 @@ | |||
#!/bin/bash | |||
#!/bin/env bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so did you actually change any logic here or just run it through some auto formatter? i dont see the point, afaik the script was working fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neither. Yes, /bin/bash
will work fine on most systems, but in rare cases this path is not correct (NixOS comes to mind). /bin/env bash
ensures it works across the board. It's just good practice.
63e0459
to
22dd307
Compare
I just released v1.6.1 to the Arch Linux extra repo. But it would be great if you could release a v1.7.0 with the latest go & dep versions.
Closes #88.