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

[WPB-14310] Move domain registration guards to their respective subsystems #4418

Merged
merged 13 commits into from
Feb 1, 2025

Conversation

akshaymankar
Copy link
Member

@akshaymankar akshaymankar commented Jan 22, 2025

https://wearezeta.atlassian.net/browse/WPB-14310

please read commit-by-commit.

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@echoes-hq echoes-hq bot added the echoes/initiative: scale Enterprise Readiness Initiatives label Jan 22, 2025
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Jan 22, 2025
@fisx fisx changed the title Move domain registration guards to their respective subsystems [WPB-14310] Move domain registration guards to their respective subsystems Jan 22, 2025
@fisx fisx force-pushed the WPB-14310-refactor-get-domain-reg branch 4 times, most recently from 153e1b3 to 50ae68a Compare January 29, 2025 16:06
@fisx fisx changed the base branch from develop to WPB-14307-new-auth January 29, 2025 16:08
@fisx fisx force-pushed the WPB-14310-refactor-get-domain-reg branch from 50ae68a to dc5e4ed Compare January 29, 2025 16:21
Comment on lines 436 to 437
. evalState []
. inMemoryDomainRegistrationStoreInterpreter
Copy link
Contributor

Choose a reason for hiding this comment

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

these two are new

@fisx fisx marked this pull request as ready for review January 29, 2025 16:22
Base automatically changed from WPB-14307-new-auth to develop January 30, 2025 13:49
@fisx fisx force-pushed the WPB-14310-refactor-get-domain-reg branch from db2ff49 to 3d1bf0e Compare January 30, 2025 13:50
@fisx fisx requested a review from battermann January 30, 2025 14:00
Copy link
Contributor

@battermann battermann left a comment

Choose a reason for hiding this comment

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

Looks good, only one marginal comment

Comment on lines +896 to +905
interp ::
Sem
'[ DRS.DomainRegistrationStore,
State [DRS.StoredDomainRegistration],
TinyLog,
Error UserSubsystemError
]
() ->
Either UserSubsystemError ()
interp = run . runError . noopLogger . evalState [] . inMemoryDomainRegistrationStoreInterpreter
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this interpreter be defined outside the test implementation, maybe?

Copy link
Contributor

Choose a reason for hiding this comment

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

not sure. i think it's too special-purpose to be moved to MockInterpreters, and it's not very long or complex. i'd prefer to keep it here?

@fisx fisx merged commit 500bfac into develop Feb 1, 2025
10 of 11 checks passed
@fisx fisx deleted the WPB-14310-refactor-get-domain-reg branch February 1, 2025 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
echoes/initiative: scale Enterprise Readiness Initiatives ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants