Skip to content
This repository has been archived by the owner on Feb 16, 2023. It is now read-only.

Make setting app info more extensible #184

Merged
merged 5 commits into from
Apr 30, 2020

Conversation

florisvdg
Copy link
Member

Passing app info to the client is now more extensible, so it can handle:

secrethub-go/0.27.0 secrethub-cli/0.37.0 secrethub-circleci-orb/1.0.0

Where the third layer comes from SECRETHUB_APP_INFO_NAME.

It can now also handle more layers of 'wrapped clients', e.g.:

secrethub-go/0.27.0 secrethub-xgo/0.1.0 secrethub-java/0.2.0 jenkins-secrethub-plugin/1.0.0

Validation

But with greater extensibility, comes greater validation: when using SECRETHUB_APP_INFO_NAME, it ignores values if they don't match [a-zA-Z0-9_-] and when explicitly setting WithAppInfo it actually errors, because as an SDK consumer you'll want to immediately know if (and why) your supplied value didn't come through.

Note: there's no validation on the app info version number at the moment, because next to the regular x.y.z versions, there also versions like commit hashes showing up sometimes and it's probably more desirable to have those come through, than have them ignored (or rejected). Agreed? Or should we see if we can find at least some layer of validation there?

@florisvdg florisvdg requested a review from jpcoenen April 26, 2020 18:36
pkg/secrethub/client.go Show resolved Hide resolved
pkg/secrethub/client.go Show resolved Hide resolved
pkg/secrethub/client.go Outdated Show resolved Hide resolved
@florisvdg florisvdg requested a review from jpcoenen April 30, 2020 12:43
pkg/secrethub/client.go Outdated Show resolved Hide resolved
pkg/secrethub/client.go Show resolved Hide resolved
Copy link
Member

@jpcoenen jpcoenen left a comment

Choose a reason for hiding this comment

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

One thingy. I trust you can make a wise decision here. So feel free to merge it afterwards.

@florisvdg florisvdg merged commit f2c311f into develop Apr 30, 2020
@florisvdg florisvdg deleted the feature/more-extensible-app-info branch April 30, 2020 14:39
@florisvdg florisvdg mentioned this pull request May 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants