Skip to content

Name the data update logging test's key so it is not an old variable name - #111

Merged
jwrosewell merged 1 commit into
rcfrom
fix/data-update-test-key-name
Nov 12, 2026
Merged

jwrosewell merged 1 commit into
rcfrom
fix/data-update-test-key-name

Conversation

@jwrosewell

@jwrosewell jwrosewell commented Nov 11, 2026 •

Copy link
Copy Markdown
Contributor

The problem

The Environment variable names check has failed on rc since
#105 merged at 19:36
UTC on 11 November 2026. On this draft's own base, rc 4bec158, it fails in
run 43115452328
with one finding, in
DataUpdateServiceLoggingTests.java line 76.

pipeline/pipeline.engines/src/test/java/fiftyone/pipeline/engines/services/DataUpdateServiceLoggingTests.java:76:         LICENSE_KEY

That test came in with
#79, merged at 19:35
as
9c17f01,
and the check with #105 a minute later as
53ccddd.
Neither draft held the other's change, so each passed alone and they clash
only together. The check runs on a draft merged with rc, so every draft
into rc now fails the same way, for example #109 in
run 43111207600.

Line 76 is the second line of a wrapped expression.

    // A license key of the length and characters the Distributor issues.
    private static final String LICENSE_KEY = "QF8ZR2KD7NMB4XWT9HJC3LPA";
    private static final String MASKED_KEY = "********************3LPA";
    private static final String KEYED_URL = UPDATE_URL + "?LicenseKeys=" +
        LICENSE_KEY + "&Download=True&Type=HashV41";

LICENSE_KEY here is a Java constant holding a made up licence key, which
the test gives the data update and then makes sure never appears in full in
the log. The test reads no environment variable at all.

What the rule says

No 51Degrees specification names environment variables. A search of
specifications at
d729b7d
for "environment variable" finds two passing mentions, in
device-detection-specification/required-examples.md line 136 and
pipeline-specification/automated-testing.md line 119. The rule 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", and the
check from #105 enforces it.
ci/check-environment-variable-names.ps1 lines 33 to 38
say what the second half of the check matches.

An old name read or set as an environment variable, or held in a constant
to fall back to. A Java read takes the name as a string, so a quoted bare
name is matched, along with the PowerShell and config forms the CI scripts
and the web example use, and a name at the start of a line past any spaces
and a list dash, which is how a workflow declares one as a key in an env:
block and how a readme lists one.

The last of those forms, ^[\t\- ]* in
line 52,
matches any line that starts with one of the three old names, which is why
line 76 fails. Line 73 has the constant after the word String, and lines
433 and 452 after an opening bracket, which no form matches.

The table in #105 removes LICENSE_KEY in favour of
_51DEGREES_LICENSE_KEY. That is the name of the variable, and this
constant is not one, so naming the constant _51DEGREES_LICENSE_KEY would
tell a reader the test reads that variable when it does not.

What changed

570f626
renames the constant to FULL_KEY on the four lines that use it, being 73,
76, 433 and 452. FULL_KEY sits beside MASKED_KEY, and the test already
speaks of the key "in full" in assertKeyOnlyMasked. The value and every
use stay the same, so nothing the test checks changes.

The check stays as it is. Its start of line form is what catches a workflow
env: key and a readme list entry, which #105 added on review in
c885993,
and a Java constant named after an old variable reads like one. In this
repository one does, because both ExamplesWithoutSettingsTest files hold
the name LicenseKey in a constant called LICENSE_KEY and pass it to
System.getenv, at
line 62 of the Device Detection one.

Leaving the check alone has a cost, which is that line 52 goes on matching any
later Java constant called RESOURCE_KEY, LICENSE_KEY or
FOD_CLOUD_API_URL that starts its own line, whichever value the constant
holds. Renaming the constant is the right answer each time, because a Java
constant taking one of the three banned names either reads that variable or
reads as though it does, and both of those are what the check is for.
Narrowing the check instead would have to tell a Java continuation line from a
workflow env: key and a readme list entry, which are the two forms the start
of line match was added for, and getting that wrong would let a real variable
through.

How CI gives this test a key

CI gives it none. The test builds its own data file configuration and passes
the constant in with setDataUpdateLicenseKeys at
line 432,
and its HTTP client is a mock, so no secret reaches it. CI sets licence keys
for other tests, as _51DEGREES_LICENSE_KEY for the 51Did tests in
cloud-51did.yml lines 78 to 85,
and as the LicenseKey system property for the examples in
ci/run-unit-tests.ps1 line 21
and
ci/run-integration-tests.ps1 line 67.
Neither changes.

Other places #79 and #105 clash

None found.

How it was tested

  • Red on rc.
    Run 43088786083,
    the push of 53ccddd, passes the check's 35 tests, reads 1,023 paths and
    fails with "1 old names are read, set or kept", naming line 76.
    Run 43115452328,
    the push of this draft's base, rc 4bec158, fails the same way over 1,024
    paths.
  • Green on this draft.
    Run 43116088612,
    on 570f626 merged with rc 4bec158, passes the 35 tests, reads 1,024 paths
    and prints "No tracked file names a variable starting with a digit or an
    old name."
  • Full build.
    Run 43116142534,
    on a temporary branch taken from rc 4bec158, succeeded at 00:08 UTC on
    12 November. Eleven jobs passed and two were skipped, those two being the
    performance comparison pair, which the temporary build turns off with
    skip-compare-performance: true. All nine Build and Test jobs, which are
    Windows and Ubuntu on Java 8,
    11, 17 and 21 and Ubuntu ARM on Java 17, printed one
    Setting 'PullRequestSha' to '570f62670259201748a7a3a16286d9944a52048d'
    line each, matching this draft's head, and Configure printed "Left out 6
    macOS entries". DataUpdateServiceLoggingTests ran in all nine, 25 tests
    with no failures and none skipped, so Make the data update follow the specification #79's tests ran over Give every environment variable one _51DEGREES_ name #105's renames.
  • Since those runs rc has taken
    #102 as
    6034fa4 and
    #109 as
    2f6ab51,
    between them the Sequence Element session id, the JavaScript Builder, the
    licence files and the configuration-validation submodule. Neither touches
    DataUpdateServiceLoggingTests.java, this draft still merges cleanly into
    rc, and the head has not moved.

What remains

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.

…name

The environment variable names check fails on rc at line 76 of
DataUpdateServiceLoggingTests.java, where the constant LICENSE_KEY starts
a wrapped line and so matches the check's start of line form for an old
name. The constant holds a made up licence key for the test rather than
naming an environment variable, so it becomes FULL_KEY, beside MASKED_KEY,
and the test reads the same.
@jwrosewell
jwrosewell marked this pull request as ready for review November 12, 2026 00:53
@jwrosewell
jwrosewell merged commit 9a66888 into rc Nov 12, 2026
3 checks passed
@jwrosewell
jwrosewell deleted the fix/data-update-test-key-name branch November 12, 2026 00:53
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.

1 participant