Skip to content

settings: generalize user-customizable env variable names (fixes #93)#135

Closed
Kunal-Darekar wants to merge 1 commit intom-lab:mainfrom
Kunal-Darekar:fix/issue-93-generalize-env-var-names
Closed

settings: generalize user-customizable env variable names (fixes #93)#135
Kunal-Darekar wants to merge 1 commit intom-lab:mainfrom
Kunal-Darekar:fix/issue-93-generalize-env-var-names

Conversation

@Kunal-Darekar
Copy link

@Kunal-Darekar Kunal-Darekar commented Mar 1, 2026

Fixes #93

The three env variables MURAKAMI_SETTINGS_LOCATION,
MURAKAMI_SETTINGS_NETWORK_TYPE, and MURAKAMI_SETTINGS_CONNECTION_TYPE
were named around a specific use case (physical location + ISP setup).
That made them confusing for users measuring something different, like
comparing networks across sites or tracking device-specific context.

Renamed to:

  • MURAKAMI_SETTINGS_DEVICE_ID
  • MURAKAMI_SETTINGS_DEVICE_METADATA1
  • MURAKAMI_SETTINGS_DEVICE_METADATA2

These names are neutral enough to work for any measurement scenario.

The rename starts at the CLI args in main.py (configargparse
auto-derives env var names from arg names, so renaming the arg is all
that's needed to change the env var). From there it cascades into
server.py, the base MurakamiRunner and MurakamiExporter classes, all
runner and exporter subclasses (which embed these values in their JSON
output as MurakamiDeviceID, MurakamiDeviceMetadata1,
MurakamiDeviceMetadata2), the config examples, the Balena provisioning
script, and the relevant docs.

All existing tests pass.

AI Assistance Disclosure: GitHub Copilot was used to help write
parts of this change. I've reviewed every line and understand what it
does.


This change is Reviewable

…evice_metadata1/device_metadata2 (issue m-lab#93)

The three user-facing env variables MURAKAMI_SETTINGS_LOCATION,
MURAKAMI_SETTINGS_NETWORK_TYPE, and MURAKAMI_SETTINGS_CONNECTION_TYPE
were too specific for general use. This renames them to
MURAKAMI_SETTINGS_DEVICE_ID, MURAKAMI_SETTINGS_DEVICE_METADATA1, and
MURAKAMI_SETTINGS_DEVICE_METADATA2 so users can define them based on
their own measurement context rather than being locked into the
location/network/connection framing.

The rename cascades from the CLI args in __main__.py through server.py,
the base runner and exporter classes, all runner and exporter subclasses
(which embed these values in their JSON output), the config examples,
the Balena setup script, and the relevant docs.

AI Assistance Disclosure: GitHub Copilot was used to assist in writing
this change. All output was reviewed and verified by the contributor.
@robertodauria robertodauria self-requested a review March 1, 2026 14:37
@robertodauria
Copy link
Collaborator

Hi @Kunal-Darekar, thanks for the PR and for the AI disclosure. However, there are some fundamental issues here:

  • Please coordinate before contributing. This issue is from 2021 and was never prioritized. Picking up a 5-year-old issue without checking with maintainers whether it's still relevant or what the desired approach is - especially for a breaking change as your first contribution - is not how we work. Please see our contribution guidelines. I'm still going to provide my feedback on the changes, but future PRs like this will be closed with no explanation.

  • Breaking changes with no migration path: This silently break every existing Murakami deployment. Anyone using MURAKAMI_SETTINGS_LOCATION, --network-type, etc. in their configs will have those values silently ignored after upgrading. For a tool deployed on remote devices (especially via Balena), this needs a migration strategy at minimum, support for the old names with a deprecation warning, and clear upgrade documentation.

  • JSON output schema change. MurakamiLocation, MurakamiNetworkType, MurakamiConnectionType are renamed in the output. Anyone parsing test results downstream (dashboards, data pipelines, BigQuery imports) will break. This also needs to be called out explicitly and handled carefully.

  • The proposed naming scheme doesn't generalize. Replacing three opinionated-but-meaningful names with device_metadata1 and device_metadata2 trades one problem for a worse one. What happens when someone needs a third metadata field?

  • --location was silently merged into --device-id. The old code had 4 fields (location, network_type, connection_type, device_id). This PR drops it to 3 by removing --location and letting the existing --device-id absorb its role.

  • Minor: In setup_balena_device.sh, the shell variables are still named $net and $conn while the usage text now says "metadata field."

Closing this PR. If you're interested in working on any issues, please start a discussion on the issue with your proposed approach (to be clear, I recommend using no AI at all for the discussion phase) before writing code.

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.

Use more general names for user-customizable env variables

2 participants