Return not found error if setting config

[spacez320]

Fixes #1783

[CLAassistant]

All committers have signed the CLA.

[rashmichandrashekar]

Is there an ETA for when this will be merged? We are waiting for this fix too. Also, what would be the release cadence for the package? Is it possible to trigger a release once this is merged?

[spacez320]

@ccoVeille Thanks for the review. Are you a maintainer, possibly able to answer @rashmichandrashekar 's question about a release?

[ccoVeille]

@ccoVeille Thanks for the review. Are you a maintainer, possibly able to answer @rashmichandrashekar 's question about a release?

Hi,

I'm not a maintainer of viper. I just reviewed thr PR because it popped out my stream of notification

[spacez320]

Paging @sagikazarmark for potential help here?

[rashmichandrashekar]

Thanks @spacez320. Gentle ping @sagikazarmark - Could you pls help get this reviewed and merged or point to someone who can help?

[spacez320]

@rashmichandrashekar In the meantime, I'm happy to maintain the branch for at least a little while if you want to start using it now.

[rashmichandrashekar]

Thanks @spacez320. I am planning to take these changes in another OSS repo kube-state-metrics, I will take your changes for testing, but would want this merged so that I can just take the next updated version in the other repo.

Pinging a few other contributors - @spf13 @alexandear @bep @bketelsen @anthonyfok @n10v - Could any one of the recent contributors please help with this? Thanks in advance

[alexandear]

Hi, I'm not a maintainer of this repository, so I can't merge.

[rashmichandrashekar]

Hi, I'm not a maintainer of this repository, so I can't merge.

Thanks! Could you pls point me to the maintainers of the repo?

[spacez320]

I sent an email to @sagikazarmark. I think we'll just have to be patient and wait for someone to respond when they have a moment. In the meantime we can also install from a fork.

[spacez320]

Just as a minor update here, am discussing with @sagikazarmark about this change and whether it should move forward, more updates soon.

In the meantime, there has actually already been a thread about this with a proposed (I think reasonable) work-around that probably still works. #1491

[sagikazarmark]

Thanks @spacez320 for putting in the work on this!

I had a few comments, can you please look at them?

Thanks!

[sagikazarmark]

Oh, almost forgot: would be nice to add some examples:

• how to handle both errors using the interface
• how to handle individual errors

Either as Go examples or in the readme. (Or maybe both)

[spacez320]

@sagikazarmark Thanks for the review.

I'm going to recommend we steer away from the interface implementation, as I'm not sure it serves enough of a purpose, for three reasons:

• I don't believe you can use normal error management things with the interface (like errors.Is or errors.As) (let me know if I'm wrong here).
• I'm guessing both finder and non-finder Viper instances in one project is a rare use case, but it just requires the user to match on separate error types denoting missing config files for each approach.
• The original issue is solved by returning the proper error: https://github.com/spf13/viper/pull/2027/files#diff-786d4c5d3482047f0ac7e62876791b9f4ba727a70acd750cf0a1bdd19c51c3f1R1493

Let me know if I convinced you or you'd still really like to add the interface.

Examples of new usage are added to the readme.

[sagikazarmark]

I don't believe you can use normal error management things with the interface (like errors.Is or errors.As) (let me know if I'm wrong here).

Not true: https://go.dev/play/p/Y-xROTywlGD

The original issue is solved by returning the proper error

AFAIK the original issue was not being able to catch both errors (search yields no result, file cannot be found), which is why people wanted the same error to be returned.

How does adding another error fix that?

[spacez320]

AFAIK the original issue was not being able to catch both errors (search yields no result, file cannot be found), which is why people wanted the same error to be returned.

My fault, I was overly fixated on solving my issue and not the more general one. New PR now tackles this too with the interface.

Thanks for the example, as well. The thing was that tripping me up is that you have to declare an interface type before you can use it, making me assume that interfaces weren't usable in errors methods without really understanding what was happening.

// This will work for structs but not interfaces
errors.As(err, &someErrorStruct{})  

// Necessary for interfaces
var someError someErrorInterface
errors.As(err &someError) 

[sagikazarmark]

Thanks @spacez320 !

I pushed some fixes to your PR.

[spacez320]

@sagikazarmark Thanks a lot for fixes. I added a tiny bit more adjustments. Hopefully it's close now.

[sagikazarmark]

There was a mixup with branch protection rules, otherwise I would have merged it.

I'll merge it once I get to a computer

[sagikazarmark]

Apologies for sitting on this for this long. I think it's in a good shape now, so LGTM.