Enable Secret Store retries #442
Conversation
![paul-the-alien[bot]](https://avatars.githubusercontent.com/in/83809?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greetings!
Thank you for contributing to this project!
If this is your first time contributing, please make
sure to read the Developer and Contributing Process guides.
Please also mind and follow our Code of Conduct.
|
If it's alright, I'd like to also implement this for other providers as I think this would be a good to have in general. |
|
I agree that makes sense to be SecretStore field. We just have to be sure to document the providers that would not support this, if we have any. Also, can you add docs for this one? |
|
Sure thing! I can also implement the logic in each of the providers. Since the underlying connection is HTTP I'm not sure it would be up to the providers to actually "support" it. |
|
Yeah, you are right. I was just thinking that for IBM we have a EnableRetries method already for that. But, yeah, it is trivial to have the logic implemented for the other ones. For gcp I think they use grpc, and that's why we had to implement the Close method, but I guess it would not be that different. |
|
I can take a look when I get off for the day and see how many I can get, and document the ones I can. |
|
We can do it one PR at a time. With docs for this one we can already merge. |
|
/ok-to-test |
I also made sure to notate that it's only in IBM for now
|
Kudos, SonarCloud Quality Gate passed!
|
|
👋 Sorry for the delay, added an example in the |
|
Hey @ADustyOldMuffin, nice PR! I've seen that no unit tests were added to cover the behavior. Can you do it? 🌟 |
|
I can do my best! |
|
@gusfcarvalho So upon further looking, I'm not sure where the tests would go and what we would actually test. Since the functionality is in the IBM http client, not here. @knelasevero Any more feedback or anything you need from me? |
|
Yeah, I am not sure how to write tests for retires. For retries to kick in we need the external api to not be reachable for a while and then be back or something like that. Not even in our e2e tests this would be trivial to implement right now, since we are only declaring the testcase structs with no custom behavior. From my side, this is approved. Gonna leave a /approve here and wait a bit to merge. @gusfcarvalho please raise any concerns if you have any :) |
|
/approve |
|
I was thinking more of a logic test for invalid inputs. I think we have already a 'weird' behavior if someone types a non-numerical string for the RetryInterval, where the IBMClient would actually exit with error (and I think it could continue without enabling the retry services). So, some unit tests + error handling can be useful =) |
|
The try interval is not supposed to be numerical its a time duration. And the int on MaxRetries should be enough. I'm also still not sure I understand what there is to unit test. |
|
You can write a test that checks if you pass something that doesn't parse well by ParseDuration you will get the expected error. And then a happy ending test with everything working not returning an error. I think @gusfcarvalho means that you can write unit tests to cover those conditionals in tests |
|
Ah okay, I think I understand now. I'll see what I can get put together. |
|
Hey, @ADustyOldMuffin do you plan on working on this? |
|
I do plan on it, but between school, work, and moving I haven't had much time to write some tests for it. |
|
Ok! No worries, just checking :) |
|
/ok-to-test sha=ce7fb16 |
|
@ADustyOldMuffin do you want us to take over the tests? I may have some time soon |
|
If you can get to it before myself then sure. I'm hoping to look at it today or tomorrow since I have some docs to update as well. I apologize been a hectic end of the year. |
|
Alright @knelasevero, let me know if that test is what we're looking for. It requires quite a bit of setup and maybe shows some general gaps in test coverage. |
|
Kudos, SonarCloud Quality Gate passed!
|
|
/approve |
|
/merge |
closes #387
This adds the field
retrySettingsto theSecretStorespec. This also implements the settings for the IBM provider.I put it in the secret store spec because I didn't see any specific reason why this couldn't be used elsewhere.