feat: open HashiCorp Vault secret provider for extension #282
Conversation
| /// <exception cref="SecretNotFoundException">The secret was not found, using the given name</exception> | ||
| public async Task<string> GetRawSecretAsync(string secretName) | ||
| /// <exception cref="ArgumentException">Thrown when the <paramref name="secretName"/> is blank.</exception> | ||
| public virtual async Task<string> GetRawSecretAsync(string secretName) |
There was a problem hiding this comment.
I'm still more a fan off dedicated / restricted extension points, but that's maybe just me.
For instance OnBefore / OnAfter overrideable methods.
Or, a default retry-mechanisms, that can be modified by the consumer.
With making the public methods virtual, you bring off course more power to t he consumer (and more responsabilities).
There was a problem hiding this comment.
I'm still more a fan off dedicated / restricted extension points, but that's maybe just me.
I see that 😄 , I think if we look for a broader approach for all Arcus types, it could be quite a job to maintain such methods as people may want to overwrite different things. Especially if you handle return values, alternate inputs... it would also become (I think) quite difficult to know where exactly the overridden call gets executed (I always have that problem when I come across such implementations).
But yeah, that could just be me 😄 .
With making the public methods virtual, you bring off course more power to t he consumer (and more responsabilities).
True, that's very true. With this, we allow more functionality to be changed, but it's also an approach we can use for every Arcus type, making it more user-friendly in my opinion. We have already received multiple requests for this, that they could override the methods. So, it seems like a logical choice.
But, if you have an alternate example of how this would work, and how we can streamline it across possible other types, feel free to show us!
There was a problem hiding this comment.
I'm still more a fan off dedicated / restricted extension points, but that's maybe just me.
For instance OnBefore / OnAfter overrideable methods.
Or, a default retry-mechanisms, that can be modified by the consumer.
I can see cases where that's useful indeed, but I mainly see those as "we allow you to do things, but the core is solely for us since it's kind of tricky and we shouldn't mess up".
While in the majority of the cases, you just want to extend what happens - But I'm fine with either, these would just add more methods but that's fine I guess.
There was a problem hiding this comment.
With regards to #282 (comment), I'd ask @fgheysels instead of me @stijnmoreels
There was a problem hiding this comment.
Actually, I do acknowledge that there are pro's and con's for both approaches:
- Controlled extension points: might be enough for 80% of the users, but it is possible that you don't cover the other 20%of the cases.
- Public virtual: you give tremendous freedom to the user of the library, but it might be easier for the consumer to mess up. On the other hand: we do not need to take developers by the hand. Disadvantage: once you do this, there's no way back.
I don't feel that this is up to me / us to decide. A discussion with users of the Arcus library or a survey can shed some more light on what the 'Arcus customers' actually want.
Co-authored-by: Frederik Gheysels <[email protected]>
|
What can be the conclusion here? @fgheysels Can I go through and merge or should there be something different. We can open a discussion where we could explore alternatives, but for now I think it's okay. |
I don't want to be the blocker if I'm the only one with a different opinion :) @tomkerkhove what is your opinion ? ps: this is also a valuable discussion on the topic. |
|
I'd suggest to move with this as-is and continue the overall discussion on arcus-azure/arcus#143 and go through all libs based on that outcome |
Make the
HashiCorpSecretProvideropen for extension by virtualizing the public secret provider's methods and give access to private members for child implementations.Relates to arcus-azure/arcus#143