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.

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

fix getRegionFromZone(), add exporter_test.go #94

Merged
merged 6 commits into from
Apr 11, 2024
Merged

Conversation

jjo
Copy link
Contributor

@jjo jjo commented Apr 11, 2024

Changes overview:

  • exporter.go:
    • Fix getRegionFromZone(), also make other functions use <provider>.ProviderName recently added (instead of fixed strings)
  • exporter_test.go:
    • Add test coverage to some functions, including the one fixed above
  • Dockerfile:
    • add --load to Dockerfile to support buildx setups.

--
Signed-off-by: JuanJo Ciarlante [email protected]

@jjo jjo requested review from paulajulve and inkel and removed request for paulajulve April 11, 2024 14:17
inkel
inkel previously approved these changes Apr 11, 2024
Copy link
Collaborator

@inkel inkel left a comment

Choose a reason for hiding this comment

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

LGTM! I just left some Go-style related comments but nothing that needs change; just left it there to:

  • spread the knowledge;
  • cannot help myself.

Makefile Show resolved Hide resolved
cmd/unused-exporter/exporter.go Outdated Show resolved Hide resolved
cmd/unused-exporter/exporter_test.go Outdated Show resolved Hide resolved
cmd/unused-exporter/exporter_test.go Outdated Show resolved Hide resolved
@inkel inkel mentioned this pull request Apr 11, 2024
paulajulve
paulajulve previously approved these changes Apr 11, 2024
Copy link
Contributor

@paulajulve paulajulve left a comment

Choose a reason for hiding this comment

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

We tested the changes to the makefile on my machine too, and it seems to be compatible with my docker setup too.

cmd/unused-exporter/exporter.go Outdated Show resolved Hide resolved
@jjo jjo dismissed stale reviews from paulajulve and inkel via 4d2c8c3 April 11, 2024 17:48
inkel
inkel previously approved these changes Apr 11, 2024
Signed-off-by: JuanJo Ciarlante <[email protected]>
@jjo
Copy link
Contributor Author

jjo commented Apr 11, 2024

About to merge, built and verified running ok (and scraping :8080/metrics with:

# AWS
docker run -v $HOME/.aws:/root/.aws:ro -p 8080:8080 us.gcr.io/kubernetes-dev/unused:v0.0.3-78-g96944e9 --aws.profile some-profile --aws.providername=EKS

# GCP
docker run -v $HOME/.config/gcloud:/root/.config/gcloud:ro -p 8080:8080 us.gcr.io/kubernetes-dev/unused:v0.0.3-78-g96944e9 --gcp.project some-project --gcp.providername=gke

@jjo jjo merged commit c2eb1aa into main Apr 11, 2024
3 checks passed
@jjo jjo deleted the jjo/fix-getregionfromzone branch April 11, 2024 18:27
@jjo
Copy link
Contributor Author

jjo commented Apr 11, 2024

built from main and pushed to us.gcr.io/kubernetes-dev/unused us.gcr.io/kubernetes-dev/unused:v0.0.3-79-gc2eb1aa

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants