Skip to content

Follow up on Web MFA TODOs#50417

Merged
Joerger merged 1 commit intomasterfrom
joerger/unify-mfa-web-follow-up
Jan 30, 2025
Merged

Follow up on Web MFA TODOs#50417
Joerger merged 1 commit intomasterfrom
joerger/unify-mfa-web-follow-up

Conversation

@Joerger
Copy link
Copy Markdown
Contributor

@Joerger Joerger commented Dec 19, 2024

Address TODOS from the recent flight of Web MFA PRs.

Depends on https://github.com/gravitational/teleport.e/pull/5727

@Joerger Joerger changed the base branch from master to joerger/web-admin-mfa-dialog December 19, 2024 03:21
@Joerger Joerger force-pushed the joerger/web-admin-mfa-dialog branch 4 times, most recently from ebade41 to b5581df Compare January 8, 2025 20:30
@Joerger Joerger force-pushed the joerger/unify-mfa-web-follow-up branch from d6e3cc6 to dd0b108 Compare January 14, 2025 20:30
@Joerger Joerger changed the base branch from joerger/web-admin-mfa-dialog to master January 14, 2025 20:30
@Joerger Joerger marked this pull request as ready for review January 14, 2025 21:14
@github-actions github-actions Bot requested review from rudream and ryanclark January 14, 2025 21:14
@Joerger Joerger added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v15 backport/branch/v17 labels Jan 15, 2025
Comment thread e
Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Contributor Author

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?

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Contributor

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.

@ryanclark
Copy link
Copy Markdown
Member

Will approve once https://github.com/gravitational/teleport.e/pull/5727 is in and e ref is updated

@Joerger Joerger force-pushed the joerger/unify-mfa-web-follow-up branch from dd0b108 to 0f831fd Compare January 27, 2025 19:35
@Joerger Joerger requested a review from ryanclark January 27, 2025 19:35
@Joerger
Copy link
Copy Markdown
Contributor Author

Joerger commented Jan 28, 2025

@ryanclark e ref now points to master

@Joerger Joerger force-pushed the joerger/unify-mfa-web-follow-up branch from 89badf5 to 94c9236 Compare January 29, 2025 19:13
@Joerger Joerger added this pull request to the merge queue Jan 30, 2025
Merged via the queue into master with commit f4514b4 Jan 30, 2025
@Joerger Joerger deleted the joerger/unify-mfa-web-follow-up branch January 30, 2025 17:52
@public-teleport-github-review-bot
Copy link
Copy Markdown

@Joerger See the table below for backport results.

Branch Result
branch/v17 Failed

Joerger added a commit that referenced this pull request Jan 30, 2025
Joerger added a commit that referenced this pull request Feb 1, 2025
github-merge-queue Bot pushed a commit that referenced this pull request Feb 1, 2025
carloscastrojumo pushed a commit to carloscastrojumo/teleport that referenced this pull request Feb 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport/branch/v17 no-changelog Indicates that a PR does not require a changelog entry size/sm ui

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants