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

Fix status check for visibility and archival #4864

Merged
merged 1 commit into from
Jun 10, 2022

Conversation

Shaddoll
Copy link
Contributor

What changed?

  • Update status check for visibility to not only check dynamic config, but also check static config
  • Update read status check for archival to not only check dynamic config, but also check static config

Why?

  • For visibility, it's possible that the dynamic config says the advanced visibility is enabled, but in static config we don't provide advanced visibility. In this case, the server might break, and we shouldn't allow that happen.
  • For archival, it's possible that the dynamic config says read is enabled, but in static config we mark read as disabled.

How did you test it?
existing test

Potential risks

Release notes

Documentation Changes

Copy link
Contributor

@davidporter-id-au davidporter-id-au left a comment

Choose a reason for hiding this comment

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

I think it's important to call out that this is preserving existing behaviour, but ty for doing this

@Shaddoll Shaddoll merged commit 494f202 into uber:master Jun 10, 2022
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