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

Make sure sentinel implementation works #39

Merged
merged 7 commits into from
Apr 19, 2016
Merged

Make sure sentinel implementation works #39

merged 7 commits into from
Apr 19, 2016

Conversation

tsteur
Copy link
Member

@tsteur tsteur commented Apr 12, 2016

refs #23

There are two fixes:

  • When saving settings via UI we use the correct backend (Sentinel or Redis) to check connection works
  • It should now actually ask sentinel for the current master server address

To use sentinel one must configure a new config setting now (sentinel_master_name)

@tsteur tsteur added this to the Current sprint milestone Apr 12, 2016
@tsteur
Copy link
Member Author

tsteur commented Apr 14, 2016

FYI: Sentinel should be configured via UI now and can accept multiple servers

@ChristianGfK
Copy link

Looks great so far!

One thing that stood out to me when I went to try it out:
All the sentinel-specific help texts on the plugin configuration page only show up after checking the sentinel checkbox and then clicking save and thus reloading the page. However, there's no indication that everything will look different once you do this.
Either forcing a save/reload right when checking the box, or at least indicating that you should click save right away after checking it, would make this more easily understandable.

Also, I guess trim()-ing the exploded hostnames/ports would probably be wise.
I bet there's going to people who will format the list as "host1, host2, host3", simply because that's the natural thing to do.

Apart from that, it seems like it's working just fine, right now. 👍

@tsteur
Copy link
Member Author

tsteur commented Apr 14, 2016

All the sentinel-specific help texts on the plugin configuration page only show up after checking the sentinel checkbox and then clicking save and thus reloading the page

Unfortunately, the Piwik Settings API doesn't let us do such things so far. I will try to make the text a little bit more clear.

Also, I guess trim()-ing the exploded hostnames/ports would probably be wise.

The values should be trimmed already and there are tests for it see eg https://github.com/piwik/plugin-QueuedTracking/pull/39/files#diff-93972b6811cf61deea59203557827f2aR191 . Was it not working?

@ChristianGfK
Copy link

ChristianGfK commented Apr 15, 2016

The values should be trimmed already and there are tests for it see eg https://github.com/piwik/plugin-QueuedTracking/pull/39/files#diff-93972b6811cf61deea59203557827f2aR191 . Was it not working?

Never mind - I just tried looking for trim() in the code quickly and didn't see it. I didn't actually check then.
I did now and it seems to work just fine. Sorry for the confusion.

@tsteur tsteur merged commit 1d67d06 into master Apr 19, 2016
@tsteur tsteur deleted the 23 branch April 19, 2016 02:22
@innocraft-automation innocraft-automation removed this from the Current sprint milestone Feb 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants