-
Notifications
You must be signed in to change notification settings - Fork 3
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 expanding default server URI #297
Conversation
Since 5fd7296 the server URL can optionally contains an ${environment} placeholder. The default URL is a build-time parameter. But in a Makefile – which debian/rules is – `$` is a special character indicating variable expansion. So previously the `${environment}` placeholder in the URL was being expanded at build time to the empty string, meaning the default server URL became `https://.metrics.endlessm.com/`. Escape the dollar sign. https://phabricator.endlessm.com/T35165
Nothing has any need to change this property from outside the class. https://phabricator.endlessm.com/T35165
This makes no functional difference but will make some future changes a little simpler. https://phabricator.endlessm.com/T35165
This just removes a couple of lines from this test. Spotted while checking that the protocol version in the URL was actually being tested. https://phabricator.endlessm.com/T35165
EmerPermissionsProvider now has a method to fetch the server URL. Make this method return the build-time default value if no value is specified in the config file, and adjust EmerDaemon to only use that method. Extend mock-permissions-provider to return a construct-time value. (It is a little unusual to return a freshly-allocated copy of a static from emer_permissions_provider_get_server_url() but that is how that function already works.) https://phabricator.endlessm.com/T35165
Previously, EmerPermissionsProvider would return the raw string value from the config file (or a built-in default value), and EmerDaemon would expand any ${environment} placeholder therein. EmerDaemon.server_url would cache the result of this expansion after the first upload, so subsequent changes to the environment (such as by changing the OSTree server URL) would not be reflected. Move expansion of the URL to EmerPermissionsProvider, and have EmerDaemon call that function each time the URL is used. The test change is necessary because of some really weird emergent behaviour of this code. Normally Unix services read their configuration files once at startup, when explicitly asked to re-read them (conventionally with SIGHUP), but not at other times; and never write to them. However, EmerPermissionsProvider is not normal: 1. It re-reads its config file every time you call a function like emer_permissions_provider_get_environment(), so its properties like EmerPermissionsProvider:enabled can change in response to you calling an accessor function! 2. Reading a property of EmerPermissionsProvider can cause the file to be written: a. If an error occurs when reading the keyfile, such as the keyfile not existing, EmerPermissionsProvider writes the defaults back to that file. b. emer_permissions_provider_get_environment() inspects the system's 'eos' OSTree remote to see if it looks like a developer system; if so, and if the configured environment is 'prod', it spontaneously changes the environment to 'dev' (and overwrites the config file). 3. The daemon has a SetEnabled() D-Bus method which is wired up to emer_permissions_provider_set_enabled(), which also writes the file. In previous incarnations of this code, calling UploadMetrics() would cause the environment to be read prior to checking whether metrics are enabled. This would in turn cause the config file to be written if it didn't previously exist. And this test is relying on that behaviour to inspect the daemon's internal state. (Interestingly the test also claimed that the daemon never reads its config once it is started, but in fact it relies on it doing so.) Now, the environment is not consulted until after checking whether metrics are enabled and determining that they are. This breaks the weird behaviour that the test is relying on. https://phabricator.endlessm.com/T35165
Hiding these multi-line strings in macros made them harder to read & edit, for no benefit.
Under some circumstances (namely when the on-disk config is missing or unparseably malformed) the default config will be written back to disk. Personally I think this is pretty weird, but if we're going to do this, we should include all the keys from the default file. https://phabricator.endlessm.com/T35165
6cafa66
to
cea6ab3
Compare
Meson 0.55 deprecated ExternalProgram.path() in favour of a new function, ExternalProgram.full_path(). The two functions are identical in behaviour, but Meson logs a "future-deprecated" warning if you use .path(). The renaming is not explained in the deprecation message or documentation, but appears to be solely for consistency with other objects. Depend on a new-enough Meson for the new .full_path() method, and use it. https://phabricator.endlessm.com/T35165
EmerDaemon listens for change notifications on EmerPermissionsProvider:daemon-enabled. Previously, it never disconnected this signal handler. In normal usage this is not a problem because the EmerPermissionsProvider is not reused for a different EmerDaemon instance. However, in the test suite it is. This manifested itself as non-deterministic segfaults. Add an assertion that makes the failure deterministic. Use g_signal_connect_object() to automatically disconnect the signal handler when the EmerDaemon object is disposed. Depend on GLib 2.74 for the G_CONNECT_DEFAULT constant, which is just a self-documenting way of writing the number 0. https://phabricator.endlessm.com/T35165
f26ab24
to
f0e91f9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read through this and nothing jumped out at me. Kudos for all the cleanups and modernizations, especially adding g_auto
and co.!
daemon/emer-daemon.c
Outdated
self->server_url = NULL; | ||
|
||
if (server_url != NULL) | ||
self->server_url = g_build_filename (server_url, CLIENT_VERSION_NUMBER "/", NULL); | ||
self->server_url = g_strdup (server_url); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I like this better anyway :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a line that was deleted in a further patch. I don't think you meant to imply that I should restore it? So I will go ahead and merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, just needlessly replying to your commit message where you noted "This makes no functional difference but will make some future changes a little simpler." ;)
The two key commits here fix regressions from #296:
The rest of this branch is a mixture of modernising emer-permissions-provider.c, and moving all handling of the server base URL there (rather than an awkward split between that file and emer-daemon.c).
https://phabricator.endlessm.com/T35165