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

[WiP] - Improving the Settings SYNC of the Primary/Replica Indices #319

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

mskapusuz
Copy link

@mskapusuz mskapusuz commented Apr 26, 2023

The PR aims prevents overriding remote Algolia Index Settings.

Key features

  • Only push a setting if the remote value is null or does not exists.
  • [to be implemented] Forcefully push a setting ability. If a setting key is defined as should be pushed forcefully override it even though it's exists and not null.
  • Reduce redundant code in Algolia_Index::get_settings methods

PR description will be updated when the PR is ready for the review.

Closes #317

@tw2113
Copy link
Member

tw2113 commented Apr 27, 2023

Hi @mskapusuz

This is looking intriguing and also a different method than I was likely originally thinking in my ongoing comments in #317, which I'm not complaining about.

Let us know if you want some early testing of things or at least once you feel it's ready for formal testing, and we can help kick the tires.

@mskapusuz
Copy link
Author

Hi @mskapusuz

This is looking intriguing and also a different method than I was likely originally thinking in my ongoing comments in #317, which I'm not complaining about.

Let us know if you want some early testing of things or at least once you feel it's ready for formal testing, and we can help kick the tires.

Hey @tw2113

Great! I will notify you as soon as it is ready for early testing. Thank you!

@mskapusuz
Copy link
Author

mskapusuz commented May 19, 2023

Hi @mskapusuz

This is looking intriguing and also a different method than I was likely originally thinking in my ongoing comments in #317, which I'm not complaining about.

Let us know if you want some early testing of things or at least once you feel it's ready for formal testing, and we can help kick the tires.

Hey @tw2113

I'm available to work on this again. The remaining things seem to:

  • An ability to push the settings as forcefully can help structural change needs in the future.
  • Code style, PHPCS

Do you think the forceful push ability can be helpful?
I would appreciate hearing back from you before moving forward.

@mskapusuz
Copy link
Author

Hey Daniel, @w3bdesign

I'm Mustafa. As I see, you've approved that. Thanks for your contribution.

@tw2113
Copy link
Member

tw2113 commented May 19, 2023

I don't have answers for the first item, but that'd be the first thing I'd suggest focusing on. I'm not as immediately worried about code style/PHPCS as I would be about functionality.

@mskapusuz
Copy link
Author

I don't have answers for the first item, but that'd be the first thing I'd suggest focusing on. I'm not as immediately worried about code style/PHPCS as I would be about functionality.

Thank you! I think that ability helps to be adapted for potential changes on the Algolia side. Therefore, I would like to implement it. I'll let you know when it's done.

@mskapusuz mskapusuz force-pushed the feat/sync-settings branch from 561c578 to 24f6bdf Compare May 19, 2023 15:40
@mskapusuz mskapusuz force-pushed the feat/sync-settings branch from 34711b4 to c8a97f8 Compare May 19, 2023 16:27
… for exceptions other than "index does not exist")
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.

Settings pushing: merge, don't blanket override
3 participants