Skip to content
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

feat(endpoints): drop id prefix req endpoint secret key #16242

Merged
merged 1 commit into from
Dec 18, 2019

Conversation

jsteenb2
Copy link
Contributor

@jsteenb2 jsteenb2 commented Dec 17, 2019

Closes #16240

Relaxes the rigid validation on endpoints to not require the id be prefixed.

@jsteenb2 jsteenb2 requested review from a team as code owners December 17, 2019 19:25
@jsteenb2 jsteenb2 requested review from a team and removed request for a team December 17, 2019 19:26
@jsteenb2 jsteenb2 force-pushed the feat/drop_id_prefix_req_endpoint_secret_key branch from 9183bb4 to aaf2ccc Compare December 17, 2019 19:26
@jsteenb2 jsteenb2 changed the title Feat/drop id prefix req endpoint secret key feat(endpoints): drop id prefix req endpoint secret key Dec 17, 2019
@jsteenb2 jsteenb2 force-pushed the feat/drop_id_prefix_req_endpoint_secret_key branch from aaf2ccc to 85edc98 Compare December 17, 2019 19:35
@@ -45,10 +45,10 @@ func (s Slack) Valid() error {
if err := s.Base.valid(); err != nil {
return err
}
if s.URL == "" && s.Token.Key == "" {
if s.URL == "" {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

token is not required for a url, if URL is "" it shoudl be an eror for slack, token shoudn't matter

@imogenkinsman
Copy link
Contributor

imogenkinsman commented Dec 17, 2019

Are we using the id prefix anywhere for lookups?

I just want to be sure that this isn't going to be a breaking change.

@jsteenb2
Copy link
Contributor Author

@jademcgough we aren't atm. We save teh entire secrets bit into the store, and never actualy use the notification id to look for the secret, the entire key is provided. It can be secret: foo and as long as that foo exists it should work

@jsteenb2 jsteenb2 force-pushed the feat/drop_id_prefix_req_endpoint_secret_key branch from 85edc98 to 9b1526b Compare December 17, 2019 21:25
the original design made the secrets unable to be reused, a bit to opinionated
to be useful eleswhere. This relaxes that requirement so that secrets can be
referenced here.
@jsteenb2 jsteenb2 force-pushed the feat/drop_id_prefix_req_endpoint_secret_key branch from 9b1526b to 10c243f Compare December 17, 2019 23:38
@jsteenb2
Copy link
Contributor Author

jsteenb2 commented Dec 18, 2019

same failing data race test from tasks/backend

@jsteenb2 jsteenb2 merged commit 38606c6 into master Dec 18, 2019
@jsteenb2 jsteenb2 deleted the feat/drop_id_prefix_req_endpoint_secret_key branch December 18, 2019 01:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

remove id prefix requirement for notification endpoint secret key value
3 participants