-
Notifications
You must be signed in to change notification settings - Fork 7
Document development conventions for config vars #620
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -403,6 +403,79 @@ In order to set up the development environment, run the following commands: | |||||||
| # Activate the conda environment | ||||||||
| conda activate birdhouse-dev | ||||||||
|
|
||||||||
|
|
||||||||
| Development Conventions | ||||||||
| ^^^^^^^^^^^^^^^^^^^^^^^ | ||||||||
|
|
||||||||
| The aim should be to **avoid** backward **incompatible** changes to decrease manual effort between each | ||||||||
| update. When the effort is low, users will be more likey to stay up to date and not "fear" updates. | ||||||||
|
|
||||||||
| We should also aim to be **user-friendly** by being consistent with the way we grow the platform and by | ||||||||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use the long-form "configuration" and "variable" Here and in all other places in the document. |
||||||||
| ======================================== | ||||||||
| * **Default value** should be in a corresponding ``default.env`` so it is easy for the user to find them. | ||||||||
| The default value should not be burried in the code. The commented out value in ``env.local.example`` | ||||||||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we need to choose which one we prefer. I would say that we should always prefer
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have always felt that all I consider the In order to avoid duplication and making I personally think |
||||||||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
(we're trying to distinguish between the "local environment file" and
Comment on lines
+427
to
+428
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As per https://github.com/bird-house/birdhouse-deploy/pull/620/files#r2666114501, I think the overloaded |
||||||||
| * Documenting in ``default.env`` is more "closer to the source" and more "together" with all other vars | ||||||||
| from the same component. Some vars/documentations are "dangerous" so we do not even expose them to | ||||||||
| ``env.local.example``. | ||||||||
|
Comment on lines
+430
to
+431
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
|
|
||||||||
| * **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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
| ============================================================ | ||||||||
| * Try to **avoid** this scenario as this is **backward incompatible** with all existing ``env.local`` on | ||||||||
| all existing deployments. | ||||||||
|
Comment on lines
+438
to
+439
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
|
|
||||||||
| * If you must, **soften the blow** by | ||||||||
|
|
||||||||
| * 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 | ||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should also mention that they should add a comment to the documentation that the old variable is now deprecated and suggest that they should update to the new way of doing things.
Comment on lines
+443
to
+444
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would rather have the convention of always using |
||||||||
|
|
||||||||
| to give time for all users to update all existing ``env.local`` on all existing deployments. | ||||||||
|
|
||||||||
| * **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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also mention that they should add an entry to
Comment on lines
+450
to
+451
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider referencing to https://github.com/bird-house/birdhouse-deploy/blob/master/birdhouse/README.rst#tagging-policy |
||||||||
|
|
||||||||
| Changing the default value for the expected format of an existing config var in ``env.local`` | ||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you mean by expected format here?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, same note about using
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||||
| ============================================================================================= | ||||||||
| * Try to **avoid** this scenario as this is **backward incompatible** with all existing ``env.local`` on | ||||||||
| all existing deployments. | ||||||||
|
Comment on lines
+455
to
+456
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
|
|
||||||||
| * Changing the format is potentially more disruptive than the default value. | ||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why? I'm not sure I understand |
||||||||
|
|
||||||||
| * If you must, **soften the blow** by | ||||||||
|
|
||||||||
| * supporting both formats in the code at the same time for a few releases or | ||||||||
| * creating a new variable for the new format so the older format is still supported by the existing var, | ||||||||
| displaying a warning that new features will only come to the new var or | ||||||||
| * in the case of default value change, document the previous default value so user can manually set the | ||||||||
| previous default value in their ``env.local`` to retain the previous behavior | ||||||||
|
|
||||||||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and in migration_guide.rst as well please |
||||||||
| as user could previously be relying on the default value and not setting the variable explicitly in their | ||||||||
| ``env.local``. If they understand the impact and wish to retain the previous behavior, they now have to | ||||||||
| explicitly set the previous default value in their ``env.local``. | ||||||||
|
|
||||||||
| * 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. | ||||||||
|
Comment on lines
+475
to
+476
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cross-ref tagging policy |
||||||||
|
|
||||||||
|
|
||||||||
| Framework tests | ||||||||
| ^^^^^^^^^^^^^^^ | ||||||||
|
|
||||||||
|
|
||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.