Repository navigation
Keep reading every renamed environment variable under its old name, deprecated, starting with FOD_CLOUD_API_URL - #108
Merged
Merged
Conversation
The cloud request engine, the 51Did client and the 51Did creator context example read the cloud endpoint from _51DEGREES_CLOUD_ENDPOINT first and, when that is unset or empty, from FOD_CLOUD_API_URL, reporting once that the old name is deprecated. Constants.FOD_CLOUD_API_URL and did_client.ENDPOINT_VARIABLE hold "FOD_CLOUD_API_URL" again and warn that they are deprecated when read. The environment variable name check leaves the old endpoint name alone, so its own tests move those lines from the names it finds to the names it leaves. Tests only. The code follows in the next commit.
… bring back the constants that held the old name The cloud request engine, the 51Did client and the 51Did creator context example read the cloud endpoint from _51DEGREES_CLOUD_ENDPOINT first. When that is unset or empty they read FOD_CLOUD_API_URL, the name the variable had in 4.5.43, and warn once with a FutureWarning that the old name is deprecated and which name replaces it. When both are set the new name wins and nothing is reported. Constants.FOD_CLOUD_API_URL is back in the cloud request engine, holding "FOD_CLOUD_API_URL", and warns with a DeprecationWarning naming Constants.CLOUD_ENDPOINT when read. did_client.ENDPOINT_VARIABLE holds "FOD_CLOUD_API_URL" again and warns in the same way, naming the new did_client.CLOUD_ENDPOINT_VARIABLE. The environment variable name check no longer fails on the old endpoint name, because the name is read again.
did_client.ENDPOINT_VARIABLE has to be found every way it was before, so the tests check dir(), the module's dictionary and a star import as well as a read by name, and that the client reads the old variable through the constant, as it did in 4.5.43. Tests only. The code follows in the next commit.
did_client.ENDPOINT_VARIABLE was served from the module's __getattr__ so that reading it warned, which took it out of the module's dictionary, so dir() and a star import no longer gave it. It is a plain attribute again, holding "FOD_CLOUD_API_URL" and marked deprecated in its comment, which points at CLOUD_ENDPOINT_VARIABLE. The client reads the old variable through the constant, as it did in 4.5.43.
…old name Every environment variable name a package, a test, an example or a CI script reads is a public name, so a name that changed to its _51DEGREES_ form is still read under the old name when the new one is unset or empty, with one message that the old name is deprecated. The tests cover the helper each test folder holds (live_cloud.py), the three example helpers and the constants they hold, the two Location cloud examples, the 51Did creator context example's two keys, the IP Intelligence test data file, and the upper case read in the two key helpers. The name check's tests say an old name is left alone in a deprecated read and in the files that may name one. Tests only. The code follows in the next commit.
…ew one is not set resource_key, license_key, cloud_endpoint and cloud_request_origin read as variables, 51DEGREES_DD_PATH, IPINTELLIGENCEDATAFILE and run_performance_tests are read again by the tests and examples that read them in 4.5.43, each only when its _51DEGREES_ name is unset or empty, and each printing once that the old name is deprecated. RESOURCE_KEY and LICENSE_KEY are read again by the 51Did creator context example. KeyUtils.get_named_key in fiftyone_devicedetection_shared and fiftyone_ipintelligence_shared reads the upper case form of a name again when the name as given is not set, with a FutureWarning. The constants in the example helpers that held an old name hold it again, marked deprecated, beside a constant for each new name. Each tox.ini passes the old names to the tests beside the new ones. The environment variable name check leaves an old name alone where it is the argument straight after its _51DEGREES_ name, and in the files that hold the deprecated constants, their tests and the tox lists.
…itch read by presence, and the name check file by file The IP Intelligence example helper read its resource key and its cloud endpoint under the old names in upper case as well, so a test sets only RESOURCE_KEY and only CLOUD_ENDPOINT. A switch such as the one that turns the performance examples on is on when its variable is set, whatever it holds, so the test helper gains tests of that under both names. The name check's tests say a listed file may hold the old names it is listed with and no others, and that any other name starting with a digit still fails there. Tests only. The code follows in the next commit.
…performance switch a presence test, and allow each file only its own old names The IP Intelligence example helper reads RESOURCE_KEY and CLOUD_ENDPOINT again, after resource_key and cloud_endpoint, as it did in 4.5.43. The test helper live_cloud.py gains is_set, which says whether a variable is set under its new name or its old one, whatever it holds. The performance examples' switch uses it, so setting the variable to an empty value still turns them on. The environment variable name check no longer leaves a listed file out whole. Each file is listed with the old names it may hold, those names alone are left alone there, and every other old name and every other name that starts with a digit still fails.
…tant Constants.FOD_CLOUD_API_URL has to be a plain attribute of its class, so every way of reading it gives the old name, and the engine has to read the old variable through it, as it did in 4.5.43, so that code which points the constant at another variable still decides what is read. The tests that reset a record of what has been reported no longer create the attribute when it is missing, so a rename of one would fail the tests. Tests only. The code follows in the next commit.
…gine reads Constants.FOD_CLOUD_API_URL was a descriptor that warned when read, and the engine read the old name from a private copy. So the class dictionary held an object where 4.5.43 held the name, and pointing the constant at another variable no longer changed what the engine read. It is a plain attribute again, holding "FOD_CLOUD_API_URL" and marked deprecated in its comment, which points at Constants.CLOUD_ENDPOINT, and the engine reads the old variable through it. The 51Did client already does the same.
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.
What was wrong
Two changes on
rc, #97 and #100, gave every environment variable one_51DEGREES_name and stopped reading the names used before. Neither has been released. A patch release must not take a public name away, and every environment variable name this repository reads is a public name, so each old name has to be read still.The names
mainat 0b4081a reads andrcat 435274d does not:mainbyFOD_CLOUD_API_URL_51DEGREES_CLOUD_ENDPOINTresource_key_51DEGREES_RESOURCE_KEYlicense_key_51DEGREES_LICENSE_KEYRESOURCE_KEY,LICENSE_KEY_51DEGREES_RESOURCE_KEY,_51DEGREES_LICENSE_KEYcloud_endpoint_51DEGREES_CLOUD_ENDPOINTRESOURCE_KEY,CLOUD_ENDPOINT_51DEGREES_RESOURCE_KEY,_51DEGREES_CLOUD_ENDPOINTcloud_request_origin_51DEGREES_CLOUD_REQUEST_ORIGIN51DEGREES_DD_PATH_51DEGREES_DD_PATHIPINTELLIGENCEDATAFILE_51DEGREES_IPI_PATHrun_performance_tests_51DEGREES_RUN_PERFORMANCE_TESTSKeyUtils.get_named_keyfiftyone_devicedetection_shared(key_utils.py line 31) andfiftyone_ipintelligence_shared(key_utils.py line 31)Four published packages are in that table. The 4.5.43 wheels of fiftyone_pipeline_cloudrequestengine and fiftyone_pipeline_did read
FOD_CLOUD_API_URL, and the 4.5.101 wheel of fiftyone_devicedetection_shared and the 4.5.2 wheel of fiftyone_ipintelligence_shared read the upper case form at line 31 ofkey_utils.py. All four were uploaded on 4 November 2026 and hold the same lines asmain. The other readers are tests and examples in this repository.Three public constants in the published packages changed as well.
Constants.FOD_CLOUD_API_URLin the Cloud Request Engine (constants.py line 25) was renamed toConstants.CLOUD_ENDPOINT, andENDPOINT_VARIABLEinfiftyone_pipeline_did.did_client(did_client.py line 77) kept its name with its value changed to"_51DEGREES_CLOUD_ENDPOINT". The example helpers lostLEGACY_RESOURCE_KEY_ENV_VARandLEGACY_LICENSE_KEY_ENV_VAR, and had the values ofENDPOINT_ENV_VAR,DATA_FILE_ENV_VAR,ORIGIN_ENV_VAR, the IP IntelligenceRESOURCE_KEY_ENV_VARand the twoUPDATE_EXAMPLE_LICENSE_KEY_NAMEchanged to the new names.After a patch upgrade, a deployment that points the engine or the 51Did client at a privately hosted cloud with
FOD_CLOUD_API_URLwould have called cloud.51degrees.com with nothing said, code that readsConstants.FOD_CLOUD_API_URLwould have raisedAttributeError, and a person who runs the tests or the examples with the names they have always set would have been told no key or no data file was set.The rules
James Rosewell, 12 November 2026, at about 11:40 UTC: "A patch release never removes or renames a public name. The old name stays for at least one release, deprecated and pointing at the new one."
Asked at about 12:05 UTC whether that covers an environment variable name a package reads as well as names in code, he answered "Yes, env names too".
Asked later the same day whether it covers environment variable names read by tests and examples as well as packages, he answered "How could they not be if they're needed for tests?", "And examples? Some people will want to run the tests themselves. That's a good thing." and "They were always public!!"
So this draft covers every environment variable name a package, a test, an example or a CI script here reads, and not only the cloud endpoint it began with. The new
_51DEGREES_names stay the names everything documents.The Cloud Request Engine specification names no environment variable for the endpoint. Its configuration table gives
EndPointthe defaulthttps://cloud.51degrees.com/api/v4/, which this change leaves alone. The 51Did package surface specification names no environment variable either.What changed
All links are to the head, 21bcbe0. Everywhere, the
_51DEGREES_name is read first, the old name is read only when the new one is unset or empty, the new name wins when both are set, and nothing else about any setting changes.The published packages
FOD_CLOUD_API_URLsecond._endpoint_from_environmentreads_51DEGREES_CLOUD_ENDPOINT, then the old name. Thecloud_endpointsetting still comes before both, at lines 162 to 167._endpoint_from_environment, called at line 530 after theendpointargument, which still comes first.warnings.warn, which is how the engine already reports (line 575, line 663 and line 695). The 51Did package reported nothing before and carries no logger, so it reports the same way. The category isFutureWarning, which the Python documentation describes as the "Base class for warnings about deprecated features when those warnings are intended for end users of applications that are written in Python", and which Python shows by default. The person who sets an environment variable runs the application, andDeprecationWarningis ignored by default outside__main__, so that person would never see one.Constants.FOD_CLOUD_API_URLis back in the Cloud Request Engine, holding"FOD_CLOUD_API_URL", at constants.py lines 27 to 32. It is a plain class attribute, and its comment carries@deprecatedand points atConstants.CLOUD_ENDPOINT. Python has no keyword that marks a constant as deprecated, and it gives no warning when read. The engine reads the old variable through the constant, at line 79, as 4.5.43 did, so code that points the constant at another variable still decides what is read. Earlier versions of this change made the constant a descriptor that gave aDeprecationWarningon each read, and had the engine read the old name from a private copy. That put an object in the class's dictionary where 4.5.43 had the name, and made the engine ignore the constant, both of which the review found.did_client.ENDPOINT_VARIABLEholds"FOD_CLOUD_API_URL"again, in the same way, as a plain module attribute at did_client.py lines 78 to 86, beside the newdid_client.CLOUD_ENDPOINT_VARIABLE, which holds"_51DEGREES_CLOUD_ENDPOINT". Its comment says it is deprecated and points at the new constant. It gives no warning when read. Making a module attribute warn when read means serving it from the module's__getattr__, which takes it out of the module's dictionary, sodir()andfrom fiftyone_pipeline_did.did_client import *stop giving it. The first version of this change did that, the review found it, and the name being found every way it was in 4.5.43 matters more than the warning. The client reads the old variable through the constant, at line 108, as 4.5.43 did.KeyUtils.get_named_keyreads the upper case form of a name again, in fiftyone_devicedetection_shared and fiftyone_ipintelligence_shared, when the name as given is unset or empty. That read is deprecated and reported once for each name with aFutureWarning. It returns what the 4.5.101 and 4.5.2 code returned for every combination of the two variables.The examples
__get_env_variable(name, old_name)in each, which prints once to the error stream that the old name is deprecated and which name replaces it. They are the Device Detection helper (lines 186 to 254), the IP Intelligence helper (lines 147 to 195, with the reader at lines 430 to 456) and the Cloud Request Engine examples' helper (lines 72 to 121). The IP Intelligence helper reads its resource key and its endpoint under the old name in upper case as well (lines 148 to 181), as 4.5.43 did. Its licence key was never read in upper case and is not now.LEGACY_RESOURCE_KEY_ENV_VARis back,ENDPOINT_ENV_VARholds"cloud_endpoint"beside the newCLOUD_ENDPOINT_ENV_VAR, andDATA_FILE_ENV_VARholds"51DEGREES_DD_PATH"beside the newDD_PATH_ENV_VAR. In the IP Intelligence helper (lines 42 to 70)RESOURCE_KEY_ENV_VARholds"resource_key"beside the newCLOUD_RESOURCE_KEY_ENV_VAR,ENDPOINT_ENV_VARholds"cloud_endpoint"besideCLOUD_ENDPOINT_ENV_VAR, andDATA_FILE_ENV_VARholds"IPINTELLIGENCEDATAFILE"besideIPI_PATH_ENV_VAR. In the Cloud Request Engine examples' helper (lines 37 to 67) the twoLEGACY_constants are back, andENDPOINT_ENV_VARandORIGIN_ENV_VARhold the old names besideCLOUD_ENDPOINT_ENV_VARandCLOUD_REQUEST_ORIGIN_ENV_VAR. Every use of a constant that meant the new name now uses the new constant, which is most of the other example files in this diff.UPDATE_EXAMPLE_LICENSE_KEY_NAMEholds"license_key"again in both, deprecated, beside the newUPDATE_EXAMPLE_LICENSE_KEY_VARIABLE. The Device Detection one reachedLICENSE_KEYin upper case throughKeyUtils, so its helper reads that as well (lines 192 to 206).resource_keysecond, through aread_environmentfunction in each (gettingstarted.py lines 41 to 68, web.py lines 41 to 68). They are scripts with no helper to share.env. Its endpoint is read at lines 139 and 140 and its keys, whose old names wereRESOURCE_KEYandLICENSE_KEY, at lines 128 and 129. It warns with aFutureWarning, as the 51Did client does.The tests
live_cloud.pygainsread_environment(name, old_name), at lines 57 to 81, andfind_resource_keyreadsresource_keywhen neither of its two new names is set (lines 97 to 106). It also gainsis_set(name, old_name)(lines 84 to 94), which says whether a variable is set under either name, whatever it holds, for a variable that works as a switch. A search for a key under other names, which the 51Did tier tests make, does not fall back. The file is in eight test folders, three of them new (the Cloud Request Engine tests, the Location tests and the IP Intelligence on-premise tests), and the eight copies are the same bytes, which a test checks.is_set, so it is on when its variable is set under either name, even to an empty value, as it was. The tests that already calledfind_resource_keyneed no change.tox.inifiles pass each old name to the tests wherever they pass its new name. tox strips every variable it is not told to keep, so without this the old names would never reach a test run under tox. pipeline/fiftyone_pipeline_did/tox.ini is one of them.The name check
ci/check-environment-variable-names.ps1still fails on an old name, except in the two places a deprecated read needs one. The first is a line where the old name is the argument straight after its_51DEGREES_name, which is how every deprecated read here is written (lines 87 to 91 and line 194). The second is a list of 21 files, each given by its exact path with the old names it may hold (lines 93 to 187). They hold the deprecated constants, the documentation that says an old name is still read, the tests that set an old name to check the read, and thetox.inilists. In such a file only the names it is listed with are left alone (lines 189 to 206). Any other old name still fails there, and so does any name that starts with a digit other than51DEGREES_DD_PATHin the two files listed with it. A newtox.iniorexample_utils.pyis on no list, so it may hold no old name.FOD_CLOUD_API_URLis in the list of old names again (line 66), so it fails in every file not listed with it. The rule that matched anyLEGACY_constant is gone, because those constants are back. The first version of this part left a listed file out of the check whole, which the review found.The two readmes changed in the first version still say
FOD_CLOUD_API_URLis deprecated and read (Cloud Request Engine, 51Did). No other readme changes, so the new names stay the ones documented.Tests
The tests went in first each time, and alone.
The tests in 1ec177ab are test_live_cloud.py (the helper under both names, reported once, and the eight copies being one file), the Device Detection and IP Intelligence and Cloud Request Engine example tests (each example helper's readers and constants, with the Device Detection key helper's upper case read), test_shared.py (the IP Intelligence key helper's upper case read), the IP Intelligence on-premise test of the data file path, the Location test (each Location example's reading function, taken out of the example's source and run on its own because importing an example calls the cloud), test_creator_context_server.py (the 51Did example's keys) and check-environment-variable-names.Tests.ps1 (the name check). 797f1834 adds to three of them, being the upper case reads in the IP Intelligence helper's tests,
is_setin test_live_cloud.py, and each file's own names in the name check's tests. 252d8871 changes the engine's test of its old constant and adds one that the engine reads through it (test_cloudrequestengine.py lines 213 to 240), and stops six tests creating the record they reset when it is missing.Red
Setting 'PullRequestSha'reading 09a9951. Four failed with'https://old.example/api/v4/' != 'https://cloud.51degrees.com/api/v4/', one with "DeprecationWarning not triggered" and one with "module 'fiftyone_pipeline_did.did_client' has no attribute 'CLOUD_ENDPOINT_VARIABLE'". The job stopped when its unit tests failed, before the integration tests, which is where the Cloud Request Engine tests run, and the other nine jobs were cancelled'http://testOldEnvVarEndPoint/' != 'https://cloud.51degrees.com/api/v4/'and one with "type object 'Constants' has no attribute 'FOD_CLOUD_API_URL'"dir()gave, and one with'https://other.example/api/v4/' != 'https://cloud.51degrees.com/api/v4/''old-key' != None,'old-resource-key' != Noneand "module 'tests.live_cloud' has no attribute 'read_environment'" (9 failed, 10 passed). The Cloud Request Engine example helper failed withassert '' == 'old-value'for each of its four settings (5 failed, 15 passed). The Device Detection helper failed with('old-value', 'old-value') != ('', '')and its key helper with'value' != None(16 failed, 6 passed, counting each case of a looped test). The IP Intelligence helper failed with('old-value', 'old-value') != (None, None)(15 failed, 4 passed), its on-premise test with'old.ipi' !=the Lite file's path (1 failed, 2 passed), and its key helper withNone != 'value'(1 failed, 4 passed). The Location examples failed because neither has aread_environmentfunction (6 failed, 3 passed)'old-value' != NoneforRESOURCE_KEYand forCLOUD_ENDPOINT(2 failed, 8 passed). The other five steps passed, as nothing they test changedtestOldEndPointConstantfailed because the class's dictionary held a descriptor object where the test wants'FOD_CLOUD_API_URL', andtestConfigEndPoint_VariableTheOldConstantNamesfailed with'http://testOldEnvVarEndPoint/' != 'https://cloud.51degrees.com/api/v4/'. The other temporary workflow passed on this commit (run 43288006475), because a test that no longer creates a missing record gives the same result while the record existsRuns 43249531675, 43260379666, 43268668697, 43279463298 and 43288007021 ran from throwaway branches, each holding the tests-only commit and one temporary workflow that installs the branch's own source and runs the named test files.
One test from 09a9951,
test_old_constant_keeps_its_old_value_and_is_deprecated, asked for a warning on read. 680d216 replaced it withtest_old_constant_keeps_its_old_valueand the two tests in its row above, and 252d887 did the same for the engine'stestOldEndPointConstant. The tests that passed before each fix are the ones that guard what must not change, such as the new name winning when both are set.Green, on the head (21bcbe0)
Setting 'PullRequestSha'line, one in each of the eleven jobs, reads 21bcbe0, and Configure printed "Left out 5 macOS entries". The new test files run inside the build as well. In the Ubuntu_Python_3.12 job the 51Did package's unit tests ran 260 passed with 8 deselected, the IP Intelligence on-premise tests 53 passed and its key helper tests 5 passed, and in the integration step the Cloud Request Engine tests ran 81 passed with 2 skipped, its examples 52 passed with 13 skipped, the Device Detection examples 41 passed, the Location tests 6 passed and the IP Intelligence examples 28 passed with 3 skippedThe earlier green runs were on earlier heads. The Cloud Request Engine endpoint tests passed 11 in run 43249636622 on 1dc955f, the 51Did client construction tests passed 14 in run 43260468270 on 468814a, and the temporary workflow passed every step in run 43269176036 on b3ccf06 and in run 43279579296 on 1b8c448. The full builds of 1dc955f (run 43249652603), 468814a (run 43260481768), b3ccf06 (run 43269213683) and 1b8c448 (run 43279596850) all passed. The runs on the head replace them.
rcmoved twice while that full build ran. #98 merged at 19:19 UTC as 58111e7 and #94 at 19:34 UTC as 5b9fd55. Every build job mergesrcinto the head before it builds, and each did so between 19:22 and 19:24 UTC, so the build ran on the head merged with 58111e7, which is why the engines package ran 65 tests where the earlier builds ran 54. #94 is not in the build. It changesjavascriptbuilder.pyand two of its tests infiftyone_pipeline_core, and #98 changesdatafile_update_service.pyand two of its tests infiftyone_pipeline_engines. This draft touches none of those six files, and GitHub reports that the head merges cleanly withrcat 5b9fd55.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.
What remains
FOD_CLOUD_API_URL, and that the examples, tests and CI scripts read only names that start_51DEGREES_. Once this merges both need to say the old names are deprecated and still read.ipintelligencetestkeyis not read again. One test infiftyone_ipintelligence_sharedset that variable itself and read it straight back to check the upper case read, so no one outside the test could set it. The test of the upper case read is back under a name that starts_51DEGREES_.LEGACY_RESOURCE_KEY_ENV_VARin the Device Detection cloud tests onmain, are not back. Those tests read the old name throughlive_cloud.pynow.rc. Onmainthey setcloud_endpointand51DEGREES_DD_PATH, and read neither.