-
Notifications
You must be signed in to change notification settings - Fork 113
Chore: Updates and Cleanup #89
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
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 🎉 |
| 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.
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.
@jonnenauha Do you still want to support 1.21 even trough go itself does not support it any longer?
| 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.
| 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
|
Pro merging and releasing this for minimizing security vulnerabilities. Love this pluging |
|
@hashworks It looks like this will not merged for unknown reason, do you maintain your own fork? |
|
No, sorry. |
eda4d7b to
3506ce7
Compare
|
Would be disappointing to not get this merged. @jonnenauha anything you need before being able to merge this? Would love to help if there are any issues |
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.