-
Notifications
You must be signed in to change notification settings - Fork 68
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
Added provider names to the config. #491
Added provider names to the config. #491
Conversation
2eba599
to
40c3e6b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a couple of small comments
de3c267
to
38e1f20
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, just one minor comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you are doing all the work on a separate feature branch, this is totally fine to merge things little by little. I think it would be nice to have in a future PR some config tests:
- checking that the service starts with multiple providers of the same type with different names
- checking that the service fails to start with multiple providers of the same type with the default name
- creating a key with a provider of type A then restarting the service with a configuration where a new provider of the type A is added but check that the key can still be accessed
38e1f20
to
0874637
Compare
This is an optional field, and will default to a suitable name for each provider if it is not provided. Two providers cannot have the same name. Closes parallaxsecond#487 Signed-off-by: Matt Davis <[email protected]>
0874637
to
8632fef
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes! We can always change things here in the future as this feature develops
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏻
This is an optional field and defaults to a suitable name for each provider if it is not provided.
Two providers cannot have the same name.
Closes #487
Signed-off-by: Matt Davis [email protected]