-
-
Notifications
You must be signed in to change notification settings - Fork 1
Change default provider to Ember #8
Conversation
@@ -25,7 +25,7 @@ jobs: | |||
tags: thegreenwebfoundation/grid-intensity-exporter:integration-test | |||
- | |||
name: Create kubernetes Kind Cluster | |||
uses: helm/kind-action@v1.1.0 | |||
uses: helm/kind-action@v1.3.0 |
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.
The integration test was failing.
This upgrades the Kubernetes version we test with from 1.19 to 1.24 and means the test will pass 🤷♂️
go.mod
Outdated
|
||
require ( | ||
github.com/cenkalti/backoff/v4 v4.1.0 | ||
github.com/prometheus/client_golang v1.8.0 | ||
github.com/thegreenwebfoundation/grid-intensity-go v0.0.0-20201120145608-ed4c07f2660c | ||
github.com/thegreenwebfoundation/grid-intensity-go v0.1.1-0.20220622112548-6b75d66a9da8 |
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.
Needs thegreenwebfoundation/grid-intensity-go#24 to be merged and released
gridIntensityRegion: {{ .Values.gridIntensity.region | quote }} | ||
{{ end }} |
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.
Region is now optional so we need this extra templating.
main.go
Outdated
if err != nil { | ||
log.Printf("failed to get carbon intensity %#v", err) | ||
} | ||
if e.region == "" { |
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 new logic to return all countries if a region is not provided.
Hey Ross, given that:
Do you think it's better to just have the exporter list a single country? The data from ember is very unlikely to change, and I can't think of a use case where you would be listing all the countries for prometheus to consume, because previously the library is been a bit like allowing a computer to say:
I'm happy to defer to you on this though, as I'm not a heavy user or grafana / prometheus. |
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.
I have one query about whether regions should be optional, but I'm prepared to follow the use cases you have in mind here Ross, and otherwise I'm happy for this to be merged in. Danke!
Good point, I'll remove the changes listing all countries. I also don't have a good use case for it right now. My thinking was about the dev experience. So the user wouldn't need to provide any configuration and could later select the country they are interested in. But I think it's better and simpler 😅 to provide example install commands with a country and the user can change to their value. |
Towards #7
Adds support for the Ember provider and makes it the default.
Or for a single country
Or using the carbonintensity.org.uk provider