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

Mirror write WIP #775

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

amcmahon-rh
Copy link
Contributor

No description provided.

@property
def should_mirror_writes(self):
print()
return False
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the overridden should_mirror_writes property with the explicit return False statement exist in CommitPhase2, not CommitPhase1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The existing values should've been the other way around. I don't know when they got switched 😖

I'm imagining some future where we add another commit phase. I think it's more likely we'll keep mirroring to phase 1, so having False be the default case for a commit phase class feels right

@crungehottman
Copy link
Member

Looks good so far! Could you please add a few tests to tests/routers/test_publish.py (or similar) to verify each case in the "Examples" section of RHELDST-28333?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants