Recreate firebase user on password reset #685
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: whaught The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
googlebot
added
the
cla: yes
whaught
changed the title
whaught
changed the title
pkg/controller/metrics.go
Outdated
| FirebaseRecreates *stats.Int64Measure | ||
| } | ||
|
|
||
| func MetricsFromContext(ctx context.Context) (context.Context, *Metrics, error) { |
There was a problem hiding this comment.
i don't love the dependency through context - it now implies an ordering between controller creation.
I still think there are other solutions that don't require that ordering / dependency
There was a problem hiding this comment.
What do you mean by ordering? This is lazily initialized as a singleton on context, if the controller can't find it on context, it gets created. Controllers can be organized in any order (the first one will create)
There was a problem hiding this comment.
it requires that everyone who calls it shares a context though, otherwise it will be created twice.
that may be OK in opencensus - I'm not positive
There was a problem hiding this comment.
I think OC actually requires you to use the ctx - all the data is stored on the ctx
|
/close I'm going to reconsider this. Not sure if we want to do this on the external login server-side - maybe we sneak this metric in the db layer to avoid mike's context-sharing concerns |
cmd/server/main.go
Outdated
| @@ -219,62 +219,64 @@ func realMain(ctx context.Context) error { | |||
| sub.Handle("/health", controller.HandleHealthz(ctx, &cfg.Database, h)).Methods("GET") | |||
| } | |||
|
|
|||
| ctx, loginController, err := login.New(ctx, firebaseInternal, auth, cfg, db, h) | |||
There was a problem hiding this comment.
That was because we were sharing context.Context so this line was unindented (to be scoped so that ctx could be shared across controllers. This has been reverted
pkg/controller/metrics.go
Outdated
| FirebaseRecreates *stats.Int64Measure | ||
| } | ||
|
|
||
| func MetricsFromContext(ctx context.Context) (context.Context, *Metrics, error) { |
There was a problem hiding this comment.
I've removed all that and shoved it into the database per our conversation. I think sharing context makes sense but @mikehelmick prefers the tighter scoping in main.go
|
/retest |
|
@whaught: The following test failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
/close This has been replaced |
|
@whaught: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Proposed Changes
Release Note