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

Make Origins readonly #393

Merged
merged 1 commit into from
Jun 26, 2023

Conversation

Regenhardt
Copy link
Contributor

Closes #392

@abergs abergs self-requested a review May 17, 2023 20:48
@abergs abergs self-assigned this May 17, 2023
@Regenhardt Regenhardt force-pushed the feature/readonly-origins branch from 82c68c2 to cf00991 Compare May 17, 2023 21:00
@codecov-commenter
Copy link

codecov-commenter commented May 17, 2023

Codecov Report

Merging #393 (4fd115c) into master (a33848c) will increase coverage by 0.05%.
The diff coverage is 25.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master     #393      +/-   ##
==========================================
+ Coverage   76.80%   76.86%   +0.05%     
==========================================
  Files          89       89              
  Lines        2531     2524       -7     
  Branches      427      426       -1     
==========================================
- Hits         1944     1940       -4     
+ Misses        466      463       -3     
  Partials      121      121              
Impacted Files Coverage Δ
Src/Fido2/AuthenticatorAssertionResponse.cs 50.84% <ø> (ø)
Src/Fido2/AuthenticatorResponse.cs 57.14% <ø> (ø)
Src/Fido2.Models/Fido2Configuration.cs 69.23% <25.00%> (+2.56%) ⬆️

@abergs abergs added the breaking change Indicate that something is a breaking change label May 19, 2023
@abergs
Copy link
Collaborator

abergs commented May 19, 2023

This solution to the #392 will cause a breaking change. That is fine since we will drop a v4 with other breaking changes.

However, looking through this code, I'm wondering if we should take the opportunity clean up some of this mess.

  • Drop the Origin property
  • Make FullyQualifiedOrigins both readonly and IReadOnlySet

That way we can just set it once in Origins setter.

Any objections @aseigler?

@aseigler
Copy link
Collaborator

No objections, makes sense to me.

@Regenhardt
Copy link
Contributor Author

Like this? Although FullyQualifiedOrigins might aswell be a { get; private set; } property now. Should I additionally document the breaking change somewhere?

@abergs
Copy link
Collaborator

abergs commented May 22, 2023

@Regenhardt It will be documented automatically in the Release notes.

I will do a final review and merge as soon as I have a moment. Traveling this week.

@abergs
Copy link
Collaborator

abergs commented Jun 21, 2023

@Regenhardt This now has a bit of conflicts because we brought some older PRs in. Would you mind rebasing and sorting it out and we're pretty good to go?

@Regenhardt Regenhardt force-pushed the feature/readonly-origins branch from 4fd115c to 7faba0e Compare June 22, 2023 09:59
@Regenhardt
Copy link
Contributor Author

Rebased it

@Regenhardt
Copy link
Contributor Author

Not sure what's going on, it looks like dotnet restore did nothing for 7 minutes and then timed out? Any idea what to do? Can the runner have had a problem so we just re-run the pipeline?

@Regenhardt Regenhardt mentioned this pull request Jun 23, 2023
@abergs
Copy link
Collaborator

abergs commented Jun 26, 2023

Re-ran all tests, seems good now.

@abergs abergs merged commit 53caf81 into passwordless-lib:master Jun 26, 2023
@Regenhardt Regenhardt deleted the feature/readonly-origins branch August 28, 2023 07:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Indicate that something is a breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

adding a origin to Origins directly should throw a error
4 participants