Repository navigation
Give every environment variable one _51DEGREES_ name - #105
Merged
Merged
Conversation
Reads every tracked file and fails on a name that starts with the digit, which a POSIX shell cannot set, and on an old name that is still read where each variable has one name. Pester tests give the check one line at a time and a workflow runs both on every pull request. This commit is the check alone, so it fails over the repository as it stands and names every place the next commit changes.
A POSIX shell cannot set a name that starts with a digit, so the three names that did move to the leading underscore, and the names read behind a _51DEGREES_ one are no longer read at all. 51DEGREES_IPI_PATH, 51DEGREES_DD_PATH and 51DEGREES_RESOURCE_KEY become _51DEGREES_IPI_PATH, _51DEGREES_DD_PATH and _51DEGREES_RESOURCE_KEY. FOD_CLOUD_API_URL becomes _51DEGREES_CLOUD_ENDPOINT, which is the name the Rust library already reads, and the constant holding it is renamed with it. The RESOURCE_KEY and LICENSE_KEY reads behind the _51DEGREES_ names in the 51Did and cloud engine examples and the live 51Did tests are removed.
The old-name half needed a quote, a dollar sign, a brace or a getenv call in front of the name, so it could not see one declared as a key in a workflow env: block or listed at the start of a line in a readme. A name at the start of a line, past any spaces, tabs and a list dash, now counts. The tests state it, with a workflow line whose key is an old name, a readme list entry and a shell assignment on the failing side, and a workflow line whose key is not an old name, and a longer name that ends in one of these words, on the passing side.
This was referenced Nov 11, 2026
Draft
The check misses an old name written as ${env:NAME}, as $ENV:NAME, as
Env:\NAME or as ${{ env.NAME }}, because its prefix set wants the dollar
and the env: adjacent, and its brace branch wants the name straight after
the brace. Both the CI scripts and the pipeline config lookup write the
braced form, so an old name can come back through the file types the check
watches.
These cases fail until the prefix set takes the env token on its own.
rc has taken a helper that reads a resource key and a licence key from several names, in the cloud request engine tests, the 51Did tests and the developer cloud engine example. The design is kept. The old names RESOURCE_KEY and LICENSE_KEY are dropped from each list, because every variable has one name and nothing is read in its place, and the workflow that runs these tests sets _51DEGREES_RESOURCE_KEY and _51DEGREES_LICENSE_KEY, so nothing in CI reads the dropped names. The cloud endpoint reads _51DEGREES_CLOUD_ENDPOINT in the example and is named that way in the test documentation.
The prefix set wanted the dollar and the env token adjacent, and its brace
branch wanted the name straight after the brace, so ${env:NAME}, $ENV:NAME,
Env:\NAME and a workflow ${{ env.NAME }} all went past. Matching the token
on its own, in any case and with a following backslash allowed, closes all
four, which is what the Go check already does.
jwrosewell
marked this pull request as ready for review
November 11, 2026 19:36
This was referenced Nov 11, 2026
Merged
Draft
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
A POSIX shell cannot set an environment variable whose name starts with a
digit, and three of this repository's names did. Measured in bash:
The readmes told a developer to set them.
examples/ip-intelligence/README.mdline 31says to "set the
51DEGREES_IPI_PATHenvironment variable or systemproperty", and
examples/device-detection/README.mdline 125says the same for
51DEGREES_DD_PATH. The only ways to set either werePowerShell,
env, or a CI definition, so the instructions could not befollowed in the shell most developers use.
Four more names were read behind a
_51DEGREES_one, so a variable had twonames and the one that answered depended on which was set.
RESOURCE_KEYandLICENSE_KEYwere read as the second argument to anenv(...)helper inCreatorContextDemoServer.javalines 122 and 124and
DidClientLiveTests.javalines 62 and 66,and as a second read in
cloud-engine/Main.javaline 80.The cloud endpoint was read as
FOD_CLOUD_API_URL, inpipeline.cloudrequestengine/.../Constants.javaline 30and
DidClient.javaline 117.What the specification says
POSIX.1-2024 (IEEE Std 1003.1-2024),
Base Definitions section 8.1, Environment Variable Definition:
Base Definitions 3.216, Name,
which is what
exportand a variable assignment take:So a process may receive a digit-initial name, but a shell can never set one.
The rest is James Rosewell's decision of 10 November 2026, quoted in
51degrees-php#67: "Just
use the version that works everywhere all the time with no fallback." Every
51Degrees environment variable name becomes
_51DEGREES_<NAME>, and nothingis read in its place.
The cloud endpoint takes the name the Rust cloud request engine already reads,
in
cloud-request-engine/src/constants.rsline 33,so one setting carries one name in every language.
What changed
Eight commits.
f5e2b65
adds the check, before the fix.
ci/check-environment-variable-names.ps1reads every file
git ls-filesreports, which leaves out build output and thesubmodules, and fails on two kinds of name.
reads an environment variable name in any case, so
51degrees_ipi_pathisthe same variable there and a POSIX shell can set neither. A name is
skipped when a name character comes before it, so
_51DEGREES_IPI_PATHpasses.
RESOURCE_KEY,LICENSE_KEYorFOD_CLOUD_API_URLwhere a variable is read, set or named.The second keeps its case, unlike the first. The cloud request engine's tests
name the query string parameter
resource_key, which differs fromRESOURCE_KEYonly by case, and a query string parameter is on the wirerather than in the environment, so matching in any case would fail on a name
that is not a variable at all. The comment above the pattern says so, and
assertEquals("resource_key=" + key, query);is one of the lines the testsrequire to pass.
The tests in
ci/check-environment-variable-names.Tests.ps1give the matcher one line at a time, and one case runs the whole check over a
small repository holding a file named
Donn+é+es.java, becausegitquotes a non-ASCII path unless told not to and a quoted path is not found on
disk. The check lists paths with
-c core.quotePath=falsefor that reason andprints how many it read and which it skipped, which are the two submodules and
the JavaScript templates directory.
.github/workflows/environment-variable-names.ymlruns the tests and then the check, on every pull request and on pushes to
mainandrc, following the pattern of the utm link lint workflow alreadyhere.
e70a841
is the fix, across 24 files.
51DEGREES_IPI_PATH_51DEGREES_IPI_PATHip-intelligence.shared/.../testhelpers/FileUtils.java:46,examples/ip-intelligence/shared/.../DataFileHelper.java:136,CountryTranslationIntegrationTest.java:53,69,OfflineProcessing.java:187,AGENTS.md:128,ip-intelligence/README.md:39,examples/ip-intelligence/README.md:31,6451DEGREES_DD_PATH_51DEGREES_DD_PATH51Degrees-OnPrem.xml:31,35, which is a config placeholder rather than aSystem.getenvcall,OnPremDataFileOptionTest.java:38,43,GettingStartedWebOnPrem.java:51,ci/run-integration-tests.ps1:168,examples/device-detection/README.md:12551DEGREES_RESOURCE_KEY_51DEGREES_RESOURCE_KEYexamples/ip-intelligence/shared/.../KeyHelper.java:38,ip-intelligence.shared/.../testhelpers/KeyUtils.java:36,KeyHelperTest.java:91,AGENTS.md:186, two readmesFOD_CLOUD_API_URL_51DEGREES_CLOUD_ENDPOINTpipeline.cloudrequestengine/.../Constants.java:30, whose constant becomesCLOUD_ENDPOINT_VARIABLE,CloudRequestEngineBuilder.java:90,95,DidClient.java:117,DidClientTests.java:133,cloud-engine/Main.java,CreatorContextDemoServer.java,pipeline/README.mdand three other readmesRESOURCE_KEY, read only when_51DEGREES_RESOURCE_KEYwas unsetCreatorContextDemoServer.java:122,135,DidClientLiveTests.java:62,cloud-engine/Main.java:80, the fodid readmeLICENSE_KEY, read only when_51DEGREES_LICENSE_KEYwas unsetCreatorContextDemoServer.java:124,DidClientLiveTests.java:66, the fodid readmeDidClient.ENDPOINT_VARIABLEsays what it is rather than repeating the name,so only its value moves. The cloud request engine's constant was named after
the variable, so it becomes
CLOUD_ENDPOINT_VARIABLE. Anyone referring toConstants.FOD_CLOUD_API_URLhas to use the new name, which is a source levelchange to a public constant and is the one thing in this draft a consumer
could notice at compile time.
IP_INTELLIGENCE_KEYin the translation module is an element data key withthe value
"ip", not a variable, andresource_keyin the cloud requestengine's tests is the query string parameter, so neither moves.
c885993
widens the check after review. The old-name half needed a quote, a dollar
sign, a brace or a
getenvcall in front of the name, so it could not see onedeclared as a key in a workflow
env:block or listed at the start of a linein a readme. A name at the start of a line, past any spaces, tabs and a list
dash, now counts. The cases added require a workflow line whose key is an old
name, a readme list entry and a shell assignment to fail, and a workflow line
whose key is not an old name to pass, so
DeviceDetection: ${{ secrets.DEVICE_DETECTION_KEY }}is left alone because asecret is read through
secrets.and notenv..The braced forms, after review
d82ff33
adds six cases that fail, and
7ebe282
makes them pass.
The prefix set wanted the dollar sign and the
envtoken next to each other,and its brace branch wanted the name straight after the brace, so a name
written with braces fell between the two and went unseen. Five lines went
past it, with the plain form that already worked at the bottom for contrast:
${env:LICENSE_KEY} = $k${Env:RESOURCE_KEY}$ENV:RESOURCE_KEY = "key"Remove-Item Env:\LICENSE_KEY${{ env.FOD_CLOUD_API_URL }}$env:LICENSE_KEY = $kThis matters here more than the shape of the hole suggests, because two of
this repository's own file types write the braced form. The CI scripts are
PowerShell, where a name is braced whenever it sits next to other text, and
the pipeline's own config lookup reads a variable as
${env:NAME}, whichFiftyOneLookupTestexercises. A check that cannot see that form would let an old name back in
through the files it exists to watch.
The fix matches the
envtoken on its own, which is what the51degrees-go check already does,
and in any letter case and with a following backslash allowed on top of that,
so
$ENV:and theEnv:drive are seen as well. Three cases pin that itstays narrow, being
${env:_51DEGREES_LICENSE_KEY}, the${env:user.home}line inFiftyOneLookupTest, and${{ env.PAID_RESOURCE_KEY }}, which is thebrowser suite's own name and not one of ours.
Taking rc
db2f8aa
merges
rc, which had moved far enough to conflict. Until it was mergedGitHub could not build a merge commit, so no check ran on this draft at all.
rchas since taken a helper that reads a resource key and a licence keyfrom several names, in
pipeline.cloudrequestengine/.../LiveCloudKeys.java,pipeline.did/.../LiveCloudKeys.javaand
cloud-engine/Main.java.That design is kept whole, including the endpoint following the key it was
issued for. Two entries leave each list, being
RESOURCE_KEYandLICENSE_KEY, because every variable has one name and nothing is read in itsplace, which is the decision quoted above and what this draft's check
enforces.
Nothing in CI sets either of the dropped names, so no run loses a key by
it. The 51Did tests are run by
.github/workflows/cloud-51did.yml,which sets
_51DEGREES_RESOURCE_KEYto each key the organisation holds inturn with
_51DEGREES_LICENSE_KEYbeside it, both still first in theirlists. The cloud request engine's live tests are not named by any workflow,
so they run with the rest in
ci/run-unit-tests.ps1, which passes-DTestResourceKeyand-DSuperResourceKeyas system properties, and bothof those names stay in the list.
ci/run-integration-tests.ps1setsneither old name anywhere.
The merge also moves the endpoint to
_51DEGREES_CLOUD_ENDPOINTin the threefiles
rchad written with the old name, two of them in test documentationand one an example read.
1db46a9
rewraps one comment in
cloud-engine/Main.javathat the longer name hadleft broken across a phrase. No code in it.
2405c68
is the head, and takes
rcagain for51degrees-java#104
and
51degrees-java#108.
The second of those matters to this draft's build rather than to its
change.
ci/run-integration-tests.ps1line 36 ran the developer exampletests with no
-DTestResourceKey, soSimpleCloudEngine_Testfailed forwant of a key on any branch that did not have the one line #108 adds. The
merge is clean, and both of the check's patterns were run over the merged
tree before pushing, with nothing found.
The build that failed for want of that key also shows the shortened name
list behaving, because
SimpleCloudEngine_Testbuilds its message fromMain.RESOURCE_KEY_NAMESrather than spelling the names out, and themessage it printed was "None of
_51DEGREES_RESOURCE_KEY,_51DEGREES_RESOURCE_KEY_BESPOKE,TestResourceKey,SUPER_RESOURCE_KEYis set". Four names rather than five, nothing asserts a count, and the four
are the ones a developer can actually set.
The old names are no longer read. Anyone who set
51DEGREES_IPI_PATH,51DEGREES_DD_PATH,51DEGREES_RESOURCE_KEY,FOD_CLOUD_API_URL,RESOURCE_KEYorLICENSE_KEYmust set the new name instead. The cloudrequest engine and the 51Did client are published packages, so a deployment
that pointed either at a privately hosted copy of the cloud with
FOD_CLOUD_API_URLhas to move to_51DEGREES_CLOUD_ENDPOINT.How it was tested
The braced forms:
on db2f8aa, the six cases before the fix. 29 tests pass and 6 fail, each
reading "Expected 'fallback' to be found in collection $null, but it was not
found", for
${env:LICENSE_KEY} = $Keys.TestResourceKey,${Env:RESOURCE_KEY},$ENV:RESOURCE_KEY = "key",Remove-Item Env:\LICENSE_KEY,<Endpoint>${env:FOD_CLOUD_API_URL}</Endpoint>and
url: ${{ env.FOD_CLOUD_API_URL }}.on 7ebe282, which is the fix. 35 tests pass and none fails, and the check
then reads 1,021 paths, skips the two submodules and the JavaScript
templates directory, and passes.
run 43071096938.
The same 35 tests pass, and the check reads 1,022 paths and passes. The
check runs on the merge of this branch with
rc, so the one extra path ispipeline/pipeline.jsonbuilder/src/test/java/fiftyone/pipeline/jsonbuilder/JsonBuilderSequenceTests.java,added to
rcby51degrees-java#104
between the two runs, and not a file of this draft.
run 43073692200,
after
rcwas taken again. 35 tests pass and the check reads the same1,022 paths and passes.
The rename itself, on the earlier heads:
Red, run 43009710661,
on f5e2b65, the check alone before the rename. Its tests pass, 21 of 21, and
the check then reads 1,004 paths and fails with "22 names start with a
digit, which a POSIX shell cannot set, so each must start with an
underscore, and 9 old names are read, set or kept".
Green, run 43019544305,
on c885993. Its tests pass, 26 of 26, and the check reads 1,009 paths and
passes.
Full build on the head 2405c68,
run 43073755228.
Passed. All 11 jobs that ran succeeded and 2 were skipped, every
Setting 'PullRequestSha'line, 10 of them, is2405c68e6ff4f3c9db4e5ad5985e127ace76486c, and Configure printed "Left out6 macOS entries".
Two earlier full builds failed, and both are named here because each says
something worth knowing.
on 1db46a9, stopped in
pipeline.enginesonDataUpdateServiceTests.DataUpdateService_Register_AutoUpdateConfiguredInterval[0]with a Mockito
TooManyActualInvocationsonfutureFactory.schedule(<any Runnable>, leq(600000)). That is a timingcheck on a scheduled task, and nothing in this draft reaches it.
on the same commit, ran that test green in every job, so the failure above
was flaky rather than a regression. It then stopped at "Pipeline developer
examples" for want of a resource key, which is the fault Give the pipeline developer examples the test resource key #108 fixes and
which
rcdid not have when that build started.Of the tests in the passing build, the one worth naming is
OnPremDataFileOptionTest,because the data file name in
51Degrees-OnPrem.xmlis resolved by thepipeline's config substitution rather than by
System.getenv, so a leadingunderscore could in principle have broken the placeholder. That test sets
_51DEGREES_DD_PATHas a system property, which substitution reads the sameway, and asserts the example picks
from-dd-path.hashahead ofTestDataFileand ahead of the Lite file. It passes, so the placeholderresolves the new name.
ci/run-integration-tests.ps1line 168 sets the same variable for theSelenium run of that example, and the live 51Did tests read the resource key
and the endpoint by their one name.
What remains
_51DEGREES_name before this merges, or it goes unread. CI sets the newnames itself.
LiveCloudKeyslists and the one incloud-engine/Main.javastillcarry
TestResourceKey,SUPER_RESOURCE_KEYandTestLicenseKeyafter thetwo old names were dropped. Those three are mixed case or otherwise outside
the convention, they came into
rcwith the live cloud tests rather thanwith this rename, and moving them needs the organisation secrets to move
with them. The same decision points at them, so they are worth a change that
can do both.
TestResourceKeyandSuperResourceKey, read throughConstants.RESOURCE_KEY_ENV_VARindevice-detection.shared/.../testhelpers/Constants.java:54andlocation/geo-location/src/test/java/fiftyone/geolocation/Constants.java:28,are the same two names in the test helpers.
KeyHelper.getOrSetResourceKey, atlines 159 to 173,
still reads
_51DEGREES_RESOURCE_KEYfirst and then the name it is passed,and its error message tells the reader that "the legacy name is also still
accepted". That is true today rather than stale, because the second name is
TestResourceKey, which this draft leaves alone for the reason above. It isnamed here because it is the one place left where a second name is both read
and advertised, and it goes when
TestResourceKeygoes. The DeviceDetection examples'
KeyHelperreads one name already.CLOUD_ROOT_URL,PAID_RESOURCE_KEY,EXAMPLE_URL,EXAMPLE_LANGandPORT, set byci/run-integration-tests.ps1lines 98 to 147, are leftalone. They are read by the shared browser test suite in
51Degrees/selenium-api-tests,
so renaming them here alone would stop that suite finding them. They move
when the suite moves, in one change across every language that runs it.
cxxsubmodule's own C and C++ examples read the IP Intelligence pathunder a digit-initial name.
51degrees-cxx#147
renames it there, and this repository takes it with the next cxx update. The
check skips the submodule, so it does not fail on that.
51degrees-java#100,
which renamed the three digit-initial names and added a JUnit guard instead of
this PowerShell one, is closed, so Java ends with one guard and this draft
carries the whole rename.
macOS was left out of this draft's full build under the 9 November decision.
The rc head is built with macOS before it merges into main.