Document development conventions for config vars #620
Conversation
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
Added development conventions for config variables in env.local.
github-actions
bot
added
the
documentation
Clarified documentation for adding and modifying config vars in env.local, emphasizing user-friendly practices and backward compatibility.
Clarify development conventions to enhance user experience and minimize update friction.
Collaborator
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/3903/Result ❌ FAILUREBIRDHOUSE_DEPLOY_BRANCH : document-dev-convention DACCS_IAC_BRANCH : master DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-216.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/579/NOTEBOOK TEST RESULTS |
Collaborator
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/3904/Result ❌ FAILUREBIRDHOUSE_DEPLOY_BRANCH : document-dev-convention DACCS_IAC_BRANCH : master DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-91.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/580/NOTEBOOK TEST RESULTS |
Collaborator
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/3905/Result ❌ FAILUREBIRDHOUSE_DEPLOY_BRANCH : document-dev-convention DACCS_IAC_BRANCH : master DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-154.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/581/NOTEBOOK TEST RESULTS |
mishaschwartz
requested changes
Dec 19, 2025
| is only an example and can not count as a default value. | ||
|
|
||
| * **Documentation** for the new var can be both in ``default.env`` and ``env.local.example`` to be | ||
| most user-friendly. If you do not wish to duplicate the info because it is big, you can put in one of |
Collaborator
There was a problem hiding this comment.
I think we need to choose which one we prefer. I would say that we should always prefer env.local.example for variables that are use facing and default.env for the internal-only variables.
Member
There was a problem hiding this comment.
I have always felt that all default.env variables are also user-facing (as in, they can be overridden, they offer customization options, and are not just internal). They are simply just not as "critical" since reasonable default are provided (as per the above aim / dev conventions).
I consider the env.local.example to be the place where platform-wide non-default/critical variables should be indicated. These include required variables that must either absolutely be set by the developer, and those that are very important to be aware of because of the scope of their impact (eg: basic functionality of components activation).
In order to avoid duplication and making env.local.example more user-friendly (due to overload of variables, we lose the importance distinction between all of them), I purposely omit most of default.env variables. If a user actually cares a lot about a certain component's "control knob options", they should dig into default.env and its respective VARS/OPTIONAL_VARS.
I personally think env.local.example still has too many unnecessary/component-specific variables (eg: don't care about THREDDS, JupyterHub, etc. if they are not enabled), as well as some deprecated components.
| the two files and reference in the other file. | ||
|
|
||
| * Documenting in ``env.local.example`` is the most user-friendly for new user starting out because they | ||
| will have to copy ``env.local.example`` to ``env.local``. |
Collaborator
There was a problem hiding this comment.
Suggested change
| will have to copy ``env.local.example`` to ``env.local``. | |
| will refer to ``env.local.example`` when writing their local environment file. |
(we're trying to distinguish between the "local environment file" and env.local which is the default location of that file)
Comment on lines
+430
to
+431
| from the same component. Some vars/documentations are "dangerous" so we do not even expose them to | ||
| ``env.local.example``. |
Collaborator
There was a problem hiding this comment.
Suggested change
| from the same component. Some vars/documentations are "dangerous" so we do not even expose them to | |
| ``env.local.example``. | |
| from the same component. Some variables are for internal use only and should not be modified by the user. This means we should not document them in ``env.local.example``. |
| * **Naming convention** should be ``<COMPONENT_NAME>_<VAR_NAME>`` to avoid name clash. For platform vars | ||
| that do not belong to any components, use ``BIRDHOUSE`` prefix instead of ``<COMPONENT_NAME>``. | ||
|
|
||
| Renaming or deleting an existing config var in ``env.local`` |
Collaborator
There was a problem hiding this comment.
Suggested change
| Renaming or deleting an existing config var in ``env.local`` | |
| Renaming or deleting an existing configuration variable |
Comment on lines
+438
to
+439
| * Try to **avoid** this scenario as this is **backward incompatible** with all existing ``env.local`` on | ||
| all existing deployments. |
Collaborator
There was a problem hiding this comment.
Suggested change
| * Try to **avoid** this scenario as this is **backward incompatible** with all existing ``env.local`` on | |
| all existing deployments. | |
| * Try to **avoid** this scenario as this is **backward incompatible** without requiring existing deployments to manually update their local environment files. |
| * **Document migration path** clearly in ``CHANGES.md``. | ||
|
|
||
| * Bump **minor** version, **not patch** version on release to signal **backward incompatible** change | ||
| requiring manual update to all existing ``env.local`` on all existing deployments. |
Collaborator
There was a problem hiding this comment.
Also mention that they should add an entry to docs/source/migration_guide.rst
| * Bump **minor** version, **not patch** version on release to signal **backward incompatible** change | ||
| requiring manual update to all existing ``env.local`` on all existing deployments. | ||
|
|
||
| Changing the default value for the expected format of an existing config var in ``env.local`` |
Collaborator
There was a problem hiding this comment.
What do you mean by expected format here?
Collaborator
There was a problem hiding this comment.
Also, same note about using env.local vs "local environment file"
Collaborator
There was a problem hiding this comment.
Can this section be combined with the one above? Maybe I'm not understanding the difference between them but it seems like the advice for both scenarios is mostly the same.
Comment on lines
+455
to
+456
| * Try to **avoid** this scenario as this is **backward incompatible** with all existing ``env.local`` on | ||
| all existing deployments. |
Collaborator
There was a problem hiding this comment.
Suggested change
| * Try to **avoid** this scenario as this is **backward incompatible** with all existing ``env.local`` on | |
| all existing deployments. | |
| * Try to **avoid** this scenario as this is **backward incompatible** without requiring existing deployments to manually update their local environment files. |
| * Try to **avoid** this scenario as this is **backward incompatible** with all existing ``env.local`` on | ||
| all existing deployments. | ||
|
|
||
| * Changing the format is potentially more disruptive than the default value. |
Collaborator
There was a problem hiding this comment.
Why? I'm not sure I understand
|
|
||
| to give time for all users to update all existing ``env.local`` on all existing deployments. | ||
|
|
||
| * **Document migration path** clearly in ``CHANGES.md``. If default value is changed, explain the impact |
Collaborator
There was a problem hiding this comment.
and in migration_guide.rst as well please
fmigneault
reviewed
Jan 6, 2026
Comment on lines
+427
to
+428
| * Documenting in ``env.local.example`` is the most user-friendly for new user starting out because they | ||
| will have to copy ``env.local.example`` to ``env.local``. |
Member
There was a problem hiding this comment.
As per https://github.com/bird-house/birdhouse-deploy/pull/620/files#r2666114501, I think the overloaded env.local.example actually produces the opposite effect of being too noisy from unnecessary definitions, which makes it less user-friendly and more daunting for new users.
fmigneault
reviewed
Jan 6, 2026
Comment on lines
+443
to
+444
| * adding to the ``BIRDHOUSE_BACKWARDS_COMPATIBLE_VARIABLES`` mapping in ``birdhouse/default.env`` or | ||
| * trying to support both names in the code at the same time for a few releases |
Member
There was a problem hiding this comment.
I would rather have the convention of always using BIRDHOUSE_BACKWARDS_COMPATIBLE_VARIABLES, and never have both names in the code. The code should only use the "final / official" name established from that change and consider the deprecated/backward-compatible name as non-existent to make code maintenance easier.
Comment on lines
+407
to
+408
| Development Conventions | ||
| ^^^^^^^^^^^^^^^^^^^^^^^ |
Member
There was a problem hiding this comment.
Seems like all of this should be in https://github.com/bird-house/birdhouse-deploy/blob/master/CONTRIBUTING.rst instead. Cross-referencing to it here should be sufficient.
Comment on lines
+450
to
+451
| * Bump **minor** version, **not patch** version on release to signal **backward incompatible** change | ||
| requiring manual update to all existing ``env.local`` on all existing deployments. |
Member
There was a problem hiding this comment.
Consider referencing to https://github.com/bird-house/birdhouse-deploy/blob/master/birdhouse/README.rst#tagging-policy
| requiring as little setup/steps (sensible default values) as possible when spin up a fresh new stack | ||
| for onboarding new users. | ||
|
|
||
| Adding a new config var to ``env.local`` |
Member
There was a problem hiding this comment.
Use the long-form "configuration" and "variable"
Here and in all other places in the document.
Comment on lines
+475
to
+476
| * Bump **minor** version, **not patch** version on release to signal **backward incompatible** change | ||
| requiring manual update to all existing ``env.local`` on all existing deployments. |
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.
Overview
Added development conventions for config variables in
env.local, focusing on minimizing update friction and enhancing user experience.Related Issue / Discussion
CI Operations
birdhouse_daccs_configs_branch: master
birdhouse_skip_ci: false