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

Make carbonifer available as Go library #41

Merged
merged 5 commits into from
Mar 3, 2023

Conversation

Joffref
Copy link
Contributor

@Joffref Joffref commented Feb 23, 2023

Closes: #40

Thanks to this PR GetEstimationFromInstanceType(), GetEstimation() and GetResource() will be available as part of carbonifer Go Lib.

Note to the maintainers: I had struggle with the way viper is used to import data, I make an ugly workaround. I'm open to your suggestion on this point in order to find the best way to patch this. 😄

@obierlaire obierlaire self-requested a review February 24, 2023 17:24
@obierlaire
Copy link
Collaborator

Thanks a lot @Joffref for this first contribution ❤️ That's much appreciated !
I haven't had time to properly review it yet, that's a nice work!
However, I've noticed a few things:

  • CI is failing: golangci-cli found one tiny error
  • Even if it's pretty straightforward GetEstimation() should have a test. It's not a big deal if you don't have time though.

The viper part does not look that ugly, I'll have a look later to see if we can find a nicer way

Again, thanks!

Signed-off-by: Mathis Joffre <mariusjoffre@gmail.com>
@Joffref
Copy link
Contributor Author

Joffref commented Feb 24, 2023

Hey @obierlaire, I've made few changes on the PR, based on your comments. Hope it will be good this time, let me know if there is something to modify in it.

Copy link
Collaborator

@obierlaire obierlaire left a comment

Choose a reason for hiding this comment

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

I think you need to rebase once #47 will be merged

pkg/estimate/estimate.go Outdated Show resolved Hide resolved
pkg/estimate/estimate.go Outdated Show resolved Hide resolved
pkg/estimate/estimate_test.go Outdated Show resolved Hide resolved
@Joffref
Copy link
Contributor Author

Joffref commented Feb 27, 2023

I think you need to rebase once #47 will be merged

I agree 👍 Waiting to the update on your side

@obierlaire
Copy link
Collaborator

#47 merged, you can rebase your branch

Joffref and others added 2 commits February 28, 2023 11:12
@Joffref Joffref requested a review from obierlaire March 2, 2023 13:35
@obierlaire
Copy link
Collaborator

@Joffref I pushed ad7d82f to your PR in order to have one single rationalized way to init Viper and set data.path variable, always from the go project root directory, and not relative to the caller. That's more DRY and seems to work...

@Joffref
Copy link
Contributor Author

Joffref commented Mar 2, 2023

Nice one ! It will be better like this 👍

@obierlaire obierlaire merged commit 6ee9739 into carboniferio:main Mar 3, 2023
@Joffref Joffref deleted the joffre/40 branch March 3, 2023 08:29
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.

Carbonifer as public Go library
2 participants