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

Extract Credential Record abstraction #1773

Merged
merged 4 commits into from
Sep 22, 2022
Merged

Extract Credential Record abstraction #1773

merged 4 commits into from
Sep 22, 2022

Conversation

emlun
Copy link
Member

@emlun emlun commented Jul 11, 2022

This is a step towards resolving #1510 and #1556, but is substantive enough to warrant review on its own. This depends on PR #1771 and is a prerequisite of PR #1774.

I open this as a "draft" PR because it has PR #1771 as its base branch, to isolate the diff to the changes unique to this PR. I will re-target this PR to the main branch before merging.


Preview | Diff

index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
Copy link
Contributor

@agl agl left a comment

Choose a reason for hiding this comment

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

LGTM with nits.

Base automatically changed from flags-namespace to main August 22, 2022 12:39
@emlun emlun marked this pull request as ready for review September 12, 2022 20:51
Copy link
Contributor

@selfissued selfissued left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@timcappalli timcappalli left a comment

Choose a reason for hiding this comment

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

lgtm

@emlun
Copy link
Member Author

emlun commented Sep 22, 2022

On the 2022-09-21 WG call we said to merge this after I'd fixed @agl's review comment, but I'd forgotten there was another unresolved comment thread. That led to a slightly more substantial change, so I'll give @timcappalli an opportunity to re-review before we merge this.

index.bs Show resolved Hide resolved
@timcappalli
Copy link
Member

changes lgtm

@emlun
Copy link
Member Author

emlun commented Sep 22, 2022

Thanks @timcappalli (and everyone else)!

@emlun emlun merged commit 6c823f1 into main Sep 22, 2022
@emlun emlun deleted the credential-record branch September 22, 2022 16:59
github-actions bot added a commit that referenced this pull request Sep 22, 2022
SHA: 6c823f1
Reason: push, by @emlun

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit to nsatragno/webauthn that referenced this pull request Sep 28, 2022
SHA: 6c823f1
Reason: push, by @nsatragno

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants