-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Follow up on Web MFA TODOs #50417
base: master
Are you sure you want to change the base?
Follow up on Web MFA TODOs #50417
Conversation
ebade41
to
b5581df
Compare
d6e3cc6
to
dd0b108
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.
I don't think the e ref should be in this PR
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.
After https://github.com/gravitational/teleport.e/pull/5727 is merged, I'll update the e ref will to master before merging. Or do you think the e ref update is a standalone PR?
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.
Hm, I thought we all normally put e ref updates into a separate PR, but I'm not sure if it really matters. Just thought it might've been accidental
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.
I thought we all normally put e ref updates into a separate PR
Usually you'd have an e PR dependent on a teleport PR, so you'd do:
- telport PR
- e PR
- teleport PR to update e Ref
But in this case the teleport PR is dependent on the e PR, so we could do:
- e PR
- teleport PR
- teleport PR to update e Ref
IMO it makes more sense to just combine the last two PRs.
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.
Yeah, usually we do OSS PR
-> e PR
-> update e-ref
because e
PR's typically need their OSS PR otherwise they'll break, since this e
PR won't break without OSS, I think it's fine to save on the extra PR and just update the e
ref here.
Just note that you'll need to change the e
ref update here after the e
PR is merged, since the e
commit hash may not be the same after it's merged into master.
Will approve once https://github.com/gravitational/teleport.e/pull/5727 is in and e ref is updated |
Address TODOS from the recent flight of Web MFA PRs.
Depends on https://github.com/gravitational/teleport.e/pull/5727
TODO update e ref to master once ^ merges