Skip to content

Refactor our data model to support masters/instances #456

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 38 commits into from
Apr 5, 2023
Merged

Refactor our data model to support masters/instances #456

merged 38 commits into from
Apr 5, 2023

Conversation

@jonahrb
Copy link
Contributor

@jonahrb jonahrb commented Mar 20, 2023 •
edited
Loading

!!REQUIRES PROTO CHANGES!!

@github-actions github-actions bot added the enhancement New features or code improvements label Mar 20, 2023
@jonahrb
Copy link
Contributor Author

jonahrb commented Mar 20, 2023

@RobPasMue starting a draft so you can monitor progress :) Give feedback incrementally as you have free time. It's going to be pretty messy...

@jonahrb
Copy link
Contributor Author

jonahrb commented Mar 20, 2023

At this point - all tests are passing with my refactoring of the Body class. More tests could be added in the future to handle more edge cases.

@jonahrb jonahrb added the PRIORITY Issue/PR assigned with this tag must be given priority over the rest of the backlog label Mar 20, 2023
@RobPasMue
Copy link
Member

RobPasMue commented Mar 21, 2023

Big PR coming up! 😄 Let's break the code

@RobPasMue
Copy link
Member

RobPasMue commented Mar 21, 2023 •
edited
Loading

My main concern at this point is inheritance (to avoid code duplication). Why can't TemplateBody and Body extend one from the other? Let's talk offline if you prefer and sum up conclusions here.

@github-actions github-actions bot added the testing Anything related to tests label Mar 23, 2023
@jonahrb jonahrb marked this pull request as ready for review March 23, 2023 19:37
@jonahrb jonahrb requested a review from RobPasMue as a code owner March 23, 2023 19:37
@RobPasMue
Copy link
Member

RobPasMue commented Mar 24, 2023

Start moving the protos over whenever you can @jonahrb. We need to generate a new server version and protos package before merging in any case.

@RobPasMue
Copy link
Member

RobPasMue commented Mar 24, 2023

I'll review the implementation on Monday morning with my laptop... Reviewing this from my phone is complicated 😄

@RobPasMue RobPasMue added the SERVER-IMPLEMENTATION Issue/PR assigned with this tag must be solved server-side label Mar 24, 2023
@github-actions github-actions bot added the maintenance Package and maintenance related label Mar 27, 2023
RobPasMue
RobPasMue requested changes Apr 4, 2023
View reviewed changes
Copy link
Member

@RobPasMue RobPasMue left a comment

Choose a reason for hiding this comment

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

Blocking merge until we talk :)

RobPasMue
RobPasMue reviewed Apr 4, 2023
View reviewed changes
@jonahrb jonahrb requested a review from RobPasMue April 4, 2023 14:25
RobPasMue
RobPasMue reviewed Apr 4, 2023
View reviewed changes
RobPasMue
RobPasMue reviewed Apr 4, 2023
View reviewed changes
RobPasMue
RobPasMue reviewed Apr 5, 2023
View reviewed changes
RobPasMue
RobPasMue reviewed Apr 5, 2023
View reviewed changes
RobPasMue
RobPasMue reviewed Apr 5, 2023
View reviewed changes
RobPasMue
RobPasMue previously approved these changes Apr 5, 2023
View reviewed changes
Copy link
Member

@RobPasMue RobPasMue left a comment

Choose a reason for hiding this comment

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

After so long... LGTM!! Thanks @jonahrb!! Great work here

@RobPasMue RobPasMue enabled auto-merge (squash) April 5, 2023 14:18
@RobPasMue RobPasMue merged commit 06dd45d into main Apr 5, 2023
@RobPasMue RobPasMue deleted the feat/refactor-for-instances branch April 5, 2023 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RobPasMue RobPasMue RobPasMue approved these changes

Assignees

@jonahrb jonahrb

Labels

enhancement New features or code improvements maintenance Package and maintenance related PRIORITY Issue/PR assigned with this tag must be given priority over the rest of the backlog SERVER-IMPLEMENTATION Issue/PR assigned with this tag must be solved server-side testing Anything related to tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jonahrb @RobPasMue