From 72ca3c63222fc67f9f07f1a241d634a4fbdec82f Mon Sep 17 00:00:00 2001 From: Sebastien Georget Date: Mon, 24 Nov 2025 17:53:34 +0100 Subject: [PATCH 1/7] Split guide into smaller parts --- CONTRIBUTING.mdt | 647 ++++++++++++++++++++++++ scripts/render.py | 21 + src/definitions/004-integration-test.md | 12 + src/definitions/100-breaking-change.md | 12 + src/definitions/200-tool.md | 4 + src/definitions/300-unit-test.md | 8 + src/definitions/400-end-to-end-test.md | 8 + src/definitions/500-smoke-test.md | 6 + 8 files changed, 718 insertions(+) create mode 100644 CONTRIBUTING.mdt create mode 100644 scripts/render.py create mode 100644 src/definitions/004-integration-test.md create mode 100644 src/definitions/100-breaking-change.md create mode 100644 src/definitions/200-tool.md create mode 100644 src/definitions/300-unit-test.md create mode 100644 src/definitions/400-end-to-end-test.md create mode 100644 src/definitions/500-smoke-test.md diff --git a/CONTRIBUTING.mdt b/CONTRIBUTING.mdt new file mode 100644 index 0000000..8129258 --- /dev/null +++ b/CONTRIBUTING.mdt @@ -0,0 +1,647 @@ +# Standards + +- [Charm Ubuntu and Python Version](#charm-ubuntu-and-python-version) +- [CI-CD](#ci-cd) +- [Downloading Binaries](#downloading-binaries) +- [Failing Status Checks](#failing-status-checks) +- [File Encoding](#file-encoding) +- [Function and Method Ordering](#function-and-method-Ordering) +- [Handling Typing Issues with python-libjuju](#handling-typing-issues-with-python-libjuju) +- [Non Compliant Code](#non-compliant-code) +- [PR comments and requests for changes](#pr-comments-and-requests-for-changes) +- [Programming Languages and Frameworks](#programming-languages-and-frameworks) +- [Random values](#random-values) +- [Repository Setup](#repository-setup) +- [Static Code Analysis](#static-code-analysis) +- [Source Code Documentation](#source-code-documentation) +- [Test Coverage](#test-coverage) +- [Test Exception Raised](#test-exception-raised) +- [Test Fixture](#test-fixture) +- [Test Structure](#test-structure) +- [Type Hints](#type-hints) + +The [Charm development best practices](https://juju.is/docs/sdk/styleguide) are +incorporated by reference. + +## Definitions + +!INCLUDE definitions/*.md + +## Programming Languages and Frameworks + +If we develop in many different programming languages and frameworks, the +maintenance cost increases, only those with knowledge of a given programming +language or framework can work on code bases in that programming language and +best practices and standards are difficult to set across the team. + +We are a team primarily focused on developing charms, and we should follow the +recommended tools and best practices of the ecosystem we work within (and help +to improve them where appropriate). As a result, the primary programming +language we work in is Python, and the primary framework we use to develop +charms with is the [Operator +Framework](https://github.com/canonical/operator). + +We will also be working on older charms which may be written in older +frameworks such as the [Reactive +Framework](https://charmsreactive.readthedocs.io/en/latest/). In some cases +we'll work on converting those to the Operator Framework but for small bug +fixes or changes this may not be the case. + +As a team we'll also be exposed to a much lesser extent to other programming +languages as a result of working with specific applications that we deploy on +Juju, or via small changes to Juju itself, which is written in Go. We also +encourage exploration and experimentation in a range of programming languages +depending on the interest of the individual as part of using Canonical's +annual training budget. + +## Charm Ubuntu and Python Version + +Using inconsistent minor Python version for development, CI and production means +that, even if tests pass during development and CI, the charm may not work in +production. + +When charms are running in production, the version of Python available to them +is dictated by the version of Ubuntu that it is running on. This is configured +in the `charmcraft.yaml` file under the `bases.build-on` and `bases.run-on` +keys. The CI should be configured to use the same version of Ubuntu as +configured under the `charmcraft.yaml` `bases.run-on` key. It is recommended +that local development be done on the version of Python that is shipped with +the Ubuntu version defined in `charmcraft.yaml` `bases.run-on`. The unit and +integration tests should be run on the same minor Python version as is shipped +with the the OS as configured under the `charmcraft.yaml` `bases.run-on` key. +With tox, for Ubuntu 22.04, this can be done using: + +``` +[testenv] +basepython = python3.10 +``` + +This ensures that the tests are run on the same Python version as the charm +will be running in production, catching any issues related to mismatched Python +versions. + +## Downloading Binaries + +Downloading binaries is only permitted from what are classed as trusted sources. +These are: + +- The Ubuntu Archives (for debian packages) +- Snaps + [owned by the "canonical" account](https://snapcraft.io/publisher/canonical) +- Snaps where the binary is built from a trusted and approved source. +- [PyPi](https://pypi.org/) + +These sources are considered trusted because we are confident that we understand +the way in which they're built, and the security commitments for packages/snaps +produced by them. At any point we can rerun our builds and know that we will +pull in the latest versions of these artefacts, and that in the case of the +first two they will include security updates that Canonical stands behind. + +Downloading binaries from any other sources and including them in our charms or +ROCKs exposes our end users to potential security issues either now or in the +future. We don't know the manner in which the binaries were built and don't have +confidence that they will be updated in the future in case of security +vulnerabilities that affect them. + +As such, we should ensure that we’re only downloading from either the trusted +sources above, or we're downloading the source instead, verifying that download +where possible, and then building that code from source on infrastructure we +trust (e.g. Launchpad builders or GitHub self-hosted runners). + +Any exception to this should be specifically noted/documented and approved by IS +Charms managers. + +## CI-CD + +The team maintains a number of repositories which are generally quite similar. +If each of the repositories maintains its own workflows, it is difficult to +introduce new tools across the repositories owned by the team. Additionally, if +shared workflows are unreliable, the team is unable to consistently merge +changes and have confidence in the CI-CD. + +Use [`operator-workflows`](https://github.com/canonical/operator-workflows) for +the CI-CD of any repositories owned by the team. Even if the repository is not a +charm, such as a GitHub action, use it to the extent possible. If a repository +has unique requirements for certain workflows, use the `operator-workflows` as +much as possible. Consider proposing changes to `operator-workflows` instead of +doing something custom in a repository. Also consider whether to make partial +use of workflows in `operator-workflows` and add anything that can't be done +with `operator-workflows` to the individual repository, if the CI-CD +requirements are unique to a repository. + +For any changes to `operator-workflows`, add tests just like we expect for any +other repository. + +Using shared workflows will make it easy to evolve the workflows used by all our +repositories and roll out new tooling and checks across them. It will also avoid +duplication in many repositories. + +Adding tests to `operator-workflows` will ensure stability of the workflows and +provide examples for how to use them. + +## Random Values + +While creating tests, sometimes you need to assign values to variables +or parameters in order to simulate a user behavior, for example. In this case, +instead of using constants or fixed values, consider using random ones generated +by [`secrets.token_hex()`](https://docs.python.org/3/library/secrets.html#secrets.token_hex). + +Reasons: + +- If you use the same fixed values in your tests every time, your tests may pass +even if there are underlying issues with your code. This can lead to false +positives and make it difficult to identify and fix real issues in your code. +- Using random values generated by secrets.token_hex() can help to prevent +collisions or conflicts between test data. +- In the case of sensitive data, if you use fixed values in your tests, there is +a risk that may be exposed or leaked, especially if your tests are run in a +shared environment. + +Example: + +```python +from secrets import token_hex + +email = token_hex(16) +``` + +## File Encoding + +If file encoding is not specified when interacting with a file, the default +value for the operating sytem is used. The default varies across operating +systems reducing the portability of code that does not specify a encoding +explicitly. See: + +For any file operations, specify the `utf-8` encoding where possible. For +example: + +```python +with open(..., encoding="utf-8") as file: + ... + +from pathlib import Path + +Path(...).read_text(encoding="utf-8") +Path(...).write_text(..., encoding="utf-8") +``` + +This ensures that the code we write is portable across operating systems. + +## Repository Setup + +The repositories that store the source code for our charms are critical to the +ongoing development of our charms. They also enforce team policies around code +review and ensure business continuity. If the repository is setup poorly, it +exposes our team and Canonical to operational risks. + +- GitHub should be used for charm source code and issue tracking. +- The repository should be publicly accessible. +- The `is-charms` team is added as maintainers. +- Management and director of the team that owns the charm are added as admins + on the repository. +- Branches are auto-deleted after merging. +- The only option for merging PRs is using a squash commit. +- The default branch is called `main`. +- The default branch is protected and can only be changed using PRs. +- The number of approvers for PRs is 2. +- Approvals are not dismissed on new commits. +- PRs can only be merged if all checks pass. +- Bypassing of the rules is disabled. +- Commits must be signed. +- [Automated secret scanning](https://docs.github.com/en/code-security/secret-scanning/configuring-secret-scanning-for-your-repositories#enabling-secret-scanning-alerts-for-users) + must be enabled. + +The above configuration ensures our team processes around changes are enforced +and provides access to the repository even if some team members are unavailable. + +The repository will contain a `CODEOWNERS` file in its root to automatically add +the `is-charms` team as reviewer + +``` +* @canonical/is-charms +``` + +## PR comments and requests for changes + +The team uses Github for reviewing changes in the codebase and integrating them +within our projects. A reviewer can comment and give feedback that potentially +leads to new changes in the submitted code. Github allows you to `request +changes` on a PR, meaning it cannot be merged until the changes are accepted. As +the team is distributed, requesting changes as a default slows down merges and +requires the reviewer to approve again. Even with sufficient approvals, a PR +cannot be merged unless the requestor accepts the changes, which might be +trivial. + +Our team favors commenting in the usual case and resorting to requesting changes +only if there is something problematic. It is adviseable to comment and let the +engineer take action rather than request changes and block the PR. The preferred +way is to comment instead. Note that committing new changes will reset +approvals either way and approvals need to be collected again, which is the +expected behavior and part of our workflow. Please note we expect the engineer +that raised the PR to deal with the comments in good faith, i.e., not just mark +them as resolved when they are not really resolved for the purpose of being able +to merge the PR. The engineer that raised the PR is responsible for closing the +comments once they are tackled. + +With commenting rather than requesting changes, an engineer gives feedback and +still allows the team (based on approvals) to decide the way forward - a change +might make it in a different PR or the change might not be desireable in the +end. The best approach is having as much feedback from as many engineers as +possible to be able to reach a sound decision. + +## Failing Status Checks + +The team uses GitHub actions to run CI which includes automated status checks +to verify the code works as expected. If PRs are merged with failing status +checks, there are potential bugs in the code which may lead to downtime or +other operational issues or a shared tool not working as expected. + +PRs can only be merged if all status checks pass. Some exceptions could +include: + +- If the team has recently taken over a new charm and changes are urgently + needed and the test suit of the existing charm is failing for spurious + reasons. +- A tool we depend on is not working, no previous working version is available + and a change is urgently needed to fix a critical issue in production. + +Even in the above cases it is not clear whether it is reasonable to merge a PR +with failing checks due to the high risks of ignoring failing checks that have +been adopted by the team. Judgement is required in these cases weighing the +risks of introducing bugs with the urgency and impact of the underlying need to +land the change. + +Alternatives to merging the PR with failing status checks include: + +- Change the code to fix the problem. +- Disable the status check (e.g., mark the test as + [`xfail`](https://docs.pytest.org/en/7.1.x/how-to/skipping.html)). This + should not be done lightly as the value the status check provides to the team + is lost. +- Wait for an upstream fix for the issue. + +If it is deemed that a change should land despite a failing status check, the +following artifacts should be added to the PR: + +- If the status check can be run in another way, e.g. locally, a copy of the run + (e.g., a copy of the terminal output) including the git commit SHA which is + being approved and the status check passing +- The reason why the PR needs to be merged without the status check passing + (e.g., because the fix is needed in production and the status check is failing + due to problems with GitHub). + +One of the repository admins should then be asked to review the artifacts and +merge the PR after completing the review. The PR should be otherwise ready to be +merged (e.g., has been approved). + +Resolving the underlying reason the status check is failing should be high +priority so that the team can rely on the automation again. + +This will ensure that we minimise the number of bugs in our code and tooling. + +## Test Structure + +Tests that are difficult to understand are of lower value because if they fail +it is difficult to understand why they fail. + +The docstring of a test has 3 sections, *arrange*, *act* and *assert*. Arrange +explains the pre-conditions required for the test, act explains what steps the +test performs and assert explains what the state must be after all actions are +complete. If the description for a section is longer than one line, any +additional lines are indented by the default indentation of the file. The test +code is separated into blocks for each section. For example: + +```Python +def test_something(): + """ + arrange: given 2 numbers + act: when they are added + assert: the result must be the sum of the 2 numbers. If a description spans + multiple lines, lines after the first should be indented by the default + spacing of the file to indicate that the description continues. + """ + num_a = 1 + num_b = 2 + + result = num_a + num_b + + assert result == 3 +``` + +Whilst this standard is usually valuable, there are cases where it imposes +unreasonable constraints. An example is complex functional tests that check +multiple interactions that require individual arrange, act, assert blocks +because they, for example, build on each other. In those cases, apply judgement +keeping in mind the option of breaking up the test into multiple tests. + +This structure makes it easy to understand what is required before test +execution, how the test works and what it checks for in the end. + +## Test Exception Raised + +Testing an exception being raised might not be enough as a type of exception +can be raised due to multiple reasons. E.g., a function could raise `ValueError` +for different arguments. + +The property of the exception being raised should be asserted against as well. +A common test would be the string representation of the exception. Using the +following `pytest` example: + +```Python +def something(a: int, b: bool): + """Do something.""" + if a >= 5: + raise ValueError("Argument a must be less than 5") + if b: + raise ValueError("Some other error message") + # Rest of the function... + + +def test_something(): + """Test the something function.""" + with pytest.raises(ValueError) as err: + something(a=10, b=False) + assert "Argument a" in str(err.value) + assert "less than 5" in str(err.value) +``` + +In the above example, the `something` function could have thrown a different +`ValueError` related to the argument `b`. Testing the string representation of +the `ValueError` ensures the proper type of error is being tested against. + +## Test Fixture + +When declaring fixture functions and requesting them in the same Python module, +there is a risk of accidentally using the fixture function declared in the +global scope instead of requesting it in the test case function parameters. + +To prevent this problem, it is recommended to add a "fixture" prefix or suffix +to the fixture function name and use the `name` argument in the `pytest.fixture` +decorator to name the fixture. + +Here's an example of using the `name` argument in `pytest.fixture`: + +```python +import pytest + +@pytest.fixture(name="app") +def app_fixture(): + return "app" + +def test_my_fixture(app): + assert app == "app" +``` + +## Test Coverage + +Unit tests check whether the intended functionality has been implemented. This +is valuable to reduce bugs and checking whether any changes to the code break +any features. The benefits of testing are roughly proportional to the test +coverage percentage, the lower the coverage the higher the chances of bugs and +the higher the risk of code changes. + +The team has a coverage percentage which is the maximum of 85% and the current +percentage on the default branch, usually `main`. Any code that is already +covered on the default branch should not cease to be covered by new commits to +main, such as through a pull request. Any coverage exclusion should have an +explanatory comment, such as: + +```Python +# Exclude from coverage since unit tests should not run as __main__ +if __name__ == "__main__": # pragma: no cover + ... +``` + +To enforce this, the `pyproject.toml` file should include the following +configuration: + +```toml +[tool.coverage.report] +fail_under = +``` + +This value should be updated in each PR to reflect any increase in coverage +compared to the main branch as a result of the PR. + +This ensures a high coverage minimum and no coverage regression. + +## Type Hints + +Python is a dynamic programming language that does not require type +declarations. Without type information, arguments might be passed to functions +that are not of the expected type (e.g., passing `None` where it is not +expected), which leads to more bugs. It also makes it more difficult to know +what functions accept as input and return as output. + +Except when impractical, declare type hints on function parameters, return +values and class and instance variables. Examples of when type hints might +be impractical (not an exhaustive list): + +- dictionaries with many nested dictionaries, +- decorator functions, +- when making small changes or +- contributions to projects not owned by the team. + +To leverage the power of type hints, the following configuration snippet should +be added to `pyproject.toml`. This helps the user during the linting process by +ensuring that all functions, including tests, have type definitions and checks +for any typing issues even if a function does not have explicit type hints on +it. + +```toml +[tool.mypy] +check_untyped_defs = true +disallow_untyped_defs = true + +[[tool.mypy.overrides]] +module = "tests.*" +disallow_untyped_defs = false +``` + +The type hints should be checked with `mypy`. More information on +type hints can be found here: [PEP 484](https://peps.python.org/pep-0484/). + +This will help users know what functions expect as parameters and return and +catches more bugs earlier. + +## Handling Typing Issues with python-libjuju + +In tests and elsewhere when interacting with `python-libjuju`, it is a frequent +requirement to check whether certain attributes are `None`. Doing this in many +tests reduces the readability of the code. + +Instead of putting `assert ops_test.model` in individual tests, write a fixture: + +```Python +@pytest_asyncio.fixture(scope="module", name="model") +async def model_fixture(ops_test: pytest_operator.plugin.OpsTest) -> ops.model.Model: + """The current test model.""" + assert ops_test.model + return ops_test.model +``` + +Instead of putting `assert hasattr(app, "units")`, write a fixture: + +```Python +@pytest_asyncio.fixture(scope="function", name="units") +async def units_fixture(app: ops.model.Application) -> list[ops.model.Unit]: + """The current test unit.""" + assert hasattr(app, "units") + return app.units +``` + +This reduces code duplication which increases the readability of the tests. + +## TODO's in source code + +Engineers tend to add TODO comments in the source code for different purposes, +e.g.: + +- To remind themselves to do something before merging a PR. +- To acknowledge that the current state of the code needs improvement, but it +depends on something external (e.g., a feature not being ready yet). +- To indicate that refactoring is necessary, but the cost is too high +- (e.g., development efforts or the PR becoming too large). + +In general, comments tend to be inaccurate because code changes faster than +comments are updated. The same applies to TODOs. +They tend to stick around forever and confuse future engineers, +who don't know what the TODO is about or if they should take action. +Therefore, TODOs should preferably be added to the product backlog +(e.g., Jira). +It is acceptable to add TODOs while developing a feature on a feature branch, +but they should be removed before merging the PR. + +## Static Code Analysis + +There are many potential problems with code that can be spotted based on +analysing source code without executing it, such as mismatches in type +expectations. Additionally, code formatting discussions during PRs can be +cumbersome and take up a lot of time. + +The following automated static code analysis tools should be used locally and +enforced through the CI system: + +- [`black`](https://pypi.org/project/black/) for code formatting + - line length of 99 + - Python target version based on the same is in the + [Charm Ubuntu and Python Version](#charm-ubuntu-and-python-version) +- [`isort`](https://pypi.org/project/isort/) for import sorting + - line length of 99 + - `black` profile +- [`flake8`](https://pypi.org/project/flake8/) for pythonic code style + - refer to + [indico `pyproject.toml`](https://github.com/canonical/indico-operator/blob/main/pyproject.toml) + - use the following additional plugins: + - `flake8-docstrings` + - `flake8-docstrings-complete` + - `flake8-test-docs` + - `flake8-copyright` + - `flake8-builtins` + - `pyproject-flake8` + - `pep8-naming` + for additional configurations +- [`bandit`](https://pypi.org/project/bandit/) for security checks +- [`codespell`](https://pypi.org/project/codespell/) for spelling problems +- [`woke`](https://snapcraft.io/woke) for inclusive language +- [`prettier`](https://prettier.io) for JSON and YAML formatting +- [`mypy`](https://pypi.org/project/mypy/) for type checks +- [`pylint`](https://pypi.org/project/pylint/) for further python code style + checks + +Note: + +- Disabling checks should be the last resort, alternatives such as refactoring + the code should be considered first. For example, instead of disabling the + `too-many-arguments` `pylint` rule, consider grouping the arguments, e.g., + using a `typing.NamedTuple`. +- When disabling a rule, if the tool allows for it, use the name of the rule + rather than the code. For example, for `pylint`, always use the name of the + rule (like `too-many-arguments`) rather than the code. This makes it easier + for readers to know which rule is being disabled and potentially why. +- If a rule is disabled, a comment should to be included above the line + disabling the rule explaining why the rule is disabled. This will mean that + future readers don't have to guess why it was disabled and can also consider + whether the disable can be removed. For rules tied to import, it may be better + to comment on each usage rather than on the import. Note that comments for + import must be added at the start of an import section to prevent the + formatter from messing up the import sections: + + ``` + # Comment explaining why subprocess is imported. + import logging + import os + import subprocess # nosec B404 + import time + ``` + +- Disabling should be done as specifically as possible. That means, disabling + the narrowest rule possible on the narrowest section of code. For example, + instead of disabling a rule entirely, disable it on a file. Instead of + disabling a rule for a file, disable it for just a line of code. The preferred + way is to disable on a line of code: + + ``` + # disable rule + ``` + + If a rule needs to be disabled for a section, re-enable it as soon as + possible: + + ``` + # disable rule + + # enable rule + ``` + +This ensures consistency across our projects, catches many potential bugs +before code is deployed and simplifies PRs. + +## Function and Method Ordering + +Without a logical order, it can be difficult to follow modules and classes as +the number of functions or methods on them grow increasing the maintenance +burden of the code. + +Functions should be ordered according to the +["step-down"](https://dzone.com/articles/the-stepdown-rule) rule. +This means that a module should be readable from top to bottom, +with functions ordered by level of abstraction, from general to specific. +A calling function should always be above the called function. +Functions should also be grouped together logically. If functions have a +similar purpose, they should be grouped together. + +On classes, the `__init__` method should come first followed by any other +factory methods, such as `from_charm`. The rest of the methods on a class should +be ordered similar to the guidance for function ordering on modules. + +This will make it easier for readers to understand the code reducing the +maintenance cost. + +## Non Compliant Code + +Standards and best practices evolve over time which means that code already +written may not comply with a new standard. If this is wide spread, it can lead +to a culture of ignoring the standard and can degreade the value of team +standards. + +At a minimum, any code changed in a pull request complies with team standards. +The team values initiative updating code to comply with new standards and +recognises that this needs to be balanced with other priorities, such as +delivering new features of fixing bugs. If possible, standards should be +enforced automatically by the CI system. + +This ensures that: + +1. code under active development is likely to be compliant with team standards, +2. less time is spent updating code that doesn't need to be updated frequently + to new standards, +3. it encourages the team to go out of their way to implement any new standards + as individuals see value in doing so and +4. compliance is enforced using CI where possible. + +## Publishing to a channel + +Charms published to tracks different from `latest` should guarantee seemlessly +upgrades between revisions, that is, revisions should not introduce breaking +changes. diff --git a/scripts/render.py b/scripts/render.py new file mode 100644 index 0000000..ab0c229 --- /dev/null +++ b/scripts/render.py @@ -0,0 +1,21 @@ +#!/usr/bin/env python3 + +from pathlib import Path + +content = Path("CONTRIBUTING.mdt") + +heading_level = 0 +for line in content.read_text().split("\n"): + if line.startswith("#"): + heading_level = len(line.split(" ")[0]) + + if not line.startswith("!INCLUDE"): + print(line) + continue + + include_pattern = line.split(" ")[1] + for included_file in sorted(Path(".").rglob(include_pattern)): + for iline in included_file.read_text().split("\n"): + if iline.startswith("#"): + iline = "#" * heading_level + iline + print(iline) diff --git a/src/definitions/004-integration-test.md b/src/definitions/004-integration-test.md new file mode 100644 index 0000000..f0c5b76 --- /dev/null +++ b/src/definitions/004-integration-test.md @@ -0,0 +1,12 @@ +# Integration Tests + +Integration tests ensure that the charm integrated with its dependencies will +behave properly. It doesn't test the code in a production like environment, +meaning that we don't connect to environments with equivalent resources and +production like datasets. The integration tests will spawn specific dependencies +that could differ from the production one (e.g using localstack instead of +openstack, sqlite instead of a production grade database, ...). These tests can +be functional (they ensure that the features provided by the charm are working +as intended) or focus on checking an abstracted interface. They don't +necessarily need to ensure that the API/Service/CLI they're interacting with are +working as intended. diff --git a/src/definitions/100-breaking-change.md b/src/definitions/100-breaking-change.md new file mode 100644 index 0000000..4289d0a --- /dev/null +++ b/src/definitions/100-breaking-change.md @@ -0,0 +1,12 @@ +# Breaking change + +A breaking change is any change that results in a deliverable that can not be +swaped by its previous version seamlessly. In the context of charms, the +following are considered breaking changes: + +* Dropping or renaming a configuration or integration +* Adding a new required configuration or integration +* Removing or changing an existing action (e.g., introducing a new required parameter) +* Introducing or modifying the structure of a secret +* Changing to an incompatible workload version +* Introducing a requirement on a juju version which was not previously required diff --git a/src/definitions/200-tool.md b/src/definitions/200-tool.md new file mode 100644 index 0000000..3d7e25a --- /dev/null +++ b/src/definitions/200-tool.md @@ -0,0 +1,4 @@ +# Tool + +A tool is a project the team works on which isn't a charm, such as a GitHub +action. diff --git a/src/definitions/300-unit-test.md b/src/definitions/300-unit-test.md new file mode 100644 index 0000000..bdcf7b5 --- /dev/null +++ b/src/definitions/300-unit-test.md @@ -0,0 +1,8 @@ +# Unit test + +These are tests that cover charm/ service functions to ensure that given a +specific context and mocked interfaces, the function returns the expected +output. Tests could cover more than one function if some of them don't include +business logic. These tests shouldn't be functional, they just ensure that the +code is doing what it's supposed to do. A test that requires too many mocks +indicates the design needs to be improved to reduce coupling. diff --git a/src/definitions/400-end-to-end-test.md b/src/definitions/400-end-to-end-test.md new file mode 100644 index 0000000..00cec60 --- /dev/null +++ b/src/definitions/400-end-to-end-test.md @@ -0,0 +1,8 @@ +# End To End Tests + +These are simulated user scenarios running on an environment as close as +possible to production. They ususally run on a staging environment with +preexisting data, and interact with the charm as a user would (ideally using the +juju cli). These are functional tests and ensure that the charm is working as +intended in the condition close to production in order to detect issues related +to this environment (ressources, pre-existing condition, migrations, ...). diff --git a/src/definitions/500-smoke-test.md b/src/definitions/500-smoke-test.md new file mode 100644 index 0000000..8d3b43f --- /dev/null +++ b/src/definitions/500-smoke-test.md @@ -0,0 +1,6 @@ +# Smoke Tests + +These are functional tests for business critical use cases used to ensure that +the most critical features are still working after a production deployment. The +aim is to ensure that a deployment didn't impact business continuity and detect +potential defects immediately after a production release. From 12b128b8ab74703d34e606091a548e041e5944a4 Mon Sep 17 00:00:00 2001 From: Sebastien Georget Date: Mon, 24 Nov 2025 18:11:07 +0100 Subject: [PATCH 2/7] Demo --- CONTRIBUTING.md | 6 +- CONTRIBUTING.mdt | 88 +++++++++---------- src/definitions/300-unit-test.md | 2 +- ...ration-test.md => 400-integration-test.md} | 0 ...-to-end-test.md => 500-end-to-end-test.md} | 0 .../{500-smoke-test.md => 600-smoke-test.md} | 0 6 files changed, 48 insertions(+), 48 deletions(-) rename src/definitions/{004-integration-test.md => 400-integration-test.md} (100%) rename src/definitions/{400-end-to-end-test.md => 500-end-to-end-test.md} (100%) rename src/definitions/{500-smoke-test.md => 600-smoke-test.md} (100%) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 78269e2..5b26fbb 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -27,9 +27,10 @@ incorporated by reference. ### Breaking change -A breaking change is any change that results in a deliverable that can not be -swaped by its previous version seamlessly. In the context of charms, the +A breaking change is any change that results in a deliverable that can not be +swaped by its previous version seamlessly. In the context of charms, the following are considered breaking changes: + * Dropping or renaming a configuration or integration * Adding a new required configuration or integration * Removing or changing an existing action (e.g., introducing a new required parameter) @@ -697,3 +698,4 @@ Charms published to tracks different from `latest` should guarantee seemlessly upgrades between revisions, that is, revisions should not introduce breaking changes. + diff --git a/CONTRIBUTING.mdt b/CONTRIBUTING.mdt index 8129258..57222a2 100644 --- a/CONTRIBUTING.mdt +++ b/CONTRIBUTING.mdt @@ -26,7 +26,6 @@ incorporated by reference. ## Definitions !INCLUDE definitions/*.md - ## Programming Languages and Frameworks If we develop in many different programming languages and frameworks, the @@ -85,11 +84,11 @@ versions. Downloading binaries is only permitted from what are classed as trusted sources. These are: -- The Ubuntu Archives (for debian packages) -- Snaps +* The Ubuntu Archives (for debian packages) +* Snaps [owned by the "canonical" account](https://snapcraft.io/publisher/canonical) -- Snaps where the binary is built from a trusted and approved source. -- [PyPi](https://pypi.org/) +* Snaps where the binary is built from a trusted and approved source. +* [PyPi](https://pypi.org/) These sources are considered trusted because we are confident that we understand the way in which they're built, and the security commitments for packages/snaps @@ -170,7 +169,7 @@ email = token_hex(16) If file encoding is not specified when interacting with a file, the default value for the operating sytem is used. The default varies across operating systems reducing the portability of code that does not specify a encoding -explicitly. See: +explicitly. See: https://peps.python.org/pep-0597/ For any file operations, specify the `utf-8` encoding where possible. For example: @@ -216,7 +215,6 @@ and provides access to the repository even if some team members are unavailable. The repository will contain a `CODEOWNERS` file in its root to automatically add the `is-charms` team as reviewer - ``` * @canonical/is-charms ``` @@ -259,10 +257,10 @@ other operational issues or a shared tool not working as expected. PRs can only be merged if all status checks pass. Some exceptions could include: -- If the team has recently taken over a new charm and changes are urgently +* If the team has recently taken over a new charm and changes are urgently needed and the test suit of the existing charm is failing for spurious reasons. -- A tool we depend on is not working, no previous working version is available +* A tool we depend on is not working, no previous working version is available and a change is urgently needed to fix a critical issue in production. Even in the above cases it is not clear whether it is reasonable to merge a PR @@ -273,20 +271,20 @@ land the change. Alternatives to merging the PR with failing status checks include: -- Change the code to fix the problem. -- Disable the status check (e.g., mark the test as +* Change the code to fix the problem. +* Disable the status check (e.g., mark the test as [`xfail`](https://docs.pytest.org/en/7.1.x/how-to/skipping.html)). This should not be done lightly as the value the status check provides to the team is lost. -- Wait for an upstream fix for the issue. +* Wait for an upstream fix for the issue. If it is deemed that a change should land despite a failing status check, the following artifacts should be added to the PR: -- If the status check can be run in another way, e.g. locally, a copy of the run +* If the status check can be run in another way, e.g. locally, a copy of the run (e.g., a copy of the terminal output) including the git commit SHA which is being approved and the status check passing -- The reason why the PR needs to be merged without the status check passing +* The reason why the PR needs to be merged without the status check passing (e.g., because the fix is needed in production and the status check is failing due to problems with GitHub). @@ -496,19 +494,18 @@ This reduces code duplication which increases the readability of the tests. Engineers tend to add TODO comments in the source code for different purposes, e.g.: - - To remind themselves to do something before merging a PR. -- To acknowledge that the current state of the code needs improvement, but it +- To acknowledge that the current state of the code needs improvement, but it depends on something external (e.g., a feature not being ready yet). -- To indicate that refactoring is necessary, but the cost is too high +- To indicate that refactoring is necessary, but the cost is too high - (e.g., development efforts or the PR becoming too large). -In general, comments tend to be inaccurate because code changes faster than -comments are updated. The same applies to TODOs. -They tend to stick around forever and confuse future engineers, -who don't know what the TODO is about or if they should take action. -Therefore, TODOs should preferably be added to the product backlog -(e.g., Jira). +In general, comments tend to be inaccurate because code changes faster than +comments are updated. The same applies to TODOs. +They tend to stick around forever and confuse future engineers, +who don't know what the TODO is about or if they should take action. +Therefore, TODOs should preferably be added to the product backlog +(e.g., Jira). It is acceptable to add TODOs while developing a feature on a feature branch, but they should be removed before merging the PR. @@ -523,23 +520,23 @@ The following automated static code analysis tools should be used locally and enforced through the CI system: - [`black`](https://pypi.org/project/black/) for code formatting - - line length of 99 - - Python target version based on the same is in the + - line length of 99 + - Python target version based on the same is in the [Charm Ubuntu and Python Version](#charm-ubuntu-and-python-version) - [`isort`](https://pypi.org/project/isort/) for import sorting - line length of 99 - `black` profile - [`flake8`](https://pypi.org/project/flake8/) for pythonic code style - - refer to + - refer to [indico `pyproject.toml`](https://github.com/canonical/indico-operator/blob/main/pyproject.toml) - - use the following additional plugins: - - `flake8-docstrings` - - `flake8-docstrings-complete` - - `flake8-test-docs` - - `flake8-copyright` - - `flake8-builtins` - - `pyproject-flake8` - - `pep8-naming` + - use the following additional plugins: + - `flake8-docstrings` + - `flake8-docstrings-complete` + - `flake8-test-docs` + - `flake8-copyright` + - `flake8-builtins` + - `pyproject-flake8` + - `pep8-naming` for additional configurations - [`bandit`](https://pypi.org/project/bandit/) for security checks - [`codespell`](https://pypi.org/project/codespell/) for spelling problems @@ -550,16 +547,15 @@ enforced through the CI system: checks Note: - -- Disabling checks should be the last resort, alternatives such as refactoring +* Disabling checks should be the last resort, alternatives such as refactoring the code should be considered first. For example, instead of disabling the `too-many-arguments` `pylint` rule, consider grouping the arguments, e.g., using a `typing.NamedTuple`. -- When disabling a rule, if the tool allows for it, use the name of the rule +* When disabling a rule, if the tool allows for it, use the name of the rule rather than the code. For example, for `pylint`, always use the name of the rule (like `too-many-arguments`) rather than the code. This makes it easier for readers to know which rule is being disabled and potentially why. -- If a rule is disabled, a comment should to be included above the line +* If a rule is disabled, a comment should to be included above the line disabling the rule explaining why the rule is disabled. This will mean that future readers don't have to guess why it was disabled and can also consider whether the disable can be removed. For rules tied to import, it may be better @@ -575,7 +571,7 @@ Note: import time ``` -- Disabling should be done as specifically as possible. That means, disabling +* Disabling should be done as specifically as possible. That means, disabling the narrowest rule possible on the narrowest section of code. For example, instead of disabling a rule entirely, disable it on a file. Instead of disabling a rule for a file, disable it for just a line of code. The preferred @@ -603,14 +599,15 @@ Without a logical order, it can be difficult to follow modules and classes as the number of functions or methods on them grow increasing the maintenance burden of the code. -Functions should be ordered according to the +Functions should be ordered according to the ["step-down"](https://dzone.com/articles/the-stepdown-rule) rule. -This means that a module should be readable from top to bottom, -with functions ordered by level of abstraction, from general to specific. -A calling function should always be above the called function. +This means that a module should be readable from top to bottom, +with functions ordered by level of abstraction, from general to specific. +A calling function should always be above the called function. Functions should also be grouped together logically. If functions have a similar purpose, they should be grouped together. + On classes, the `__init__` method should come first followed by any other factory methods, such as `from_charm`. The rest of the methods on a class should be ordered similar to the guidance for function ordering on modules. @@ -642,6 +639,7 @@ This ensures that: ## Publishing to a channel -Charms published to tracks different from `latest` should guarantee seemlessly -upgrades between revisions, that is, revisions should not introduce breaking +Charms published to tracks different from `latest` should guarantee seemlessly +upgrades between revisions, that is, revisions should not introduce breaking changes. + diff --git a/src/definitions/300-unit-test.md b/src/definitions/300-unit-test.md index bdcf7b5..102943f 100644 --- a/src/definitions/300-unit-test.md +++ b/src/definitions/300-unit-test.md @@ -1,4 +1,4 @@ -# Unit test +# Unit Tests These are tests that cover charm/ service functions to ensure that given a specific context and mocked interfaces, the function returns the expected diff --git a/src/definitions/004-integration-test.md b/src/definitions/400-integration-test.md similarity index 100% rename from src/definitions/004-integration-test.md rename to src/definitions/400-integration-test.md diff --git a/src/definitions/400-end-to-end-test.md b/src/definitions/500-end-to-end-test.md similarity index 100% rename from src/definitions/400-end-to-end-test.md rename to src/definitions/500-end-to-end-test.md diff --git a/src/definitions/500-smoke-test.md b/src/definitions/600-smoke-test.md similarity index 100% rename from src/definitions/500-smoke-test.md rename to src/definitions/600-smoke-test.md From 502129957625b31c536daf7493004a6d04db2a46 Mon Sep 17 00:00:00 2001 From: Sebastien Georget Date: Tue, 13 Jan 2026 16:53:57 +0100 Subject: [PATCH 3/7] Split guidelines to ease maintenance --- CONTRIBUTING.md | 84 ++- CONTRIBUTING.mdt | 617 +----------------- scripts/render.py | 12 +- src/ci/100-CI-CD.md | 27 + src/ci/200-failing-checks.md | 49 ++ src/github/100-repository-setup.md | 33 + src/github/200-prs.md | 27 + src/implementation/100-random-values.md | 25 + src/implementation/200-encoding.md | 21 + src/implementation/300-typing.md | 66 ++ src/implementation/400-todos.md | 19 + .../500-function-and-method-ordering.md | 20 + src/implementation/600-non-compliant-code.md | 21 + ...00-programming-languages-and-frameworks.md | 26 + src/principles/200-versions.md | 25 + src/principles/300-binaries.md | 30 + src/publication/100-publishing.md | 5 + src/tests/100-structure.md | 37 ++ src/tests/200-exceptions.md | 31 + src/tests/300-fixtures.md | 22 + src/tests/400-coverage.md | 32 + src/tests/500-static-code-analysis.md | 84 +++ 22 files changed, 681 insertions(+), 632 deletions(-) create mode 100644 src/ci/100-CI-CD.md create mode 100644 src/ci/200-failing-checks.md create mode 100644 src/github/100-repository-setup.md create mode 100644 src/github/200-prs.md create mode 100644 src/implementation/100-random-values.md create mode 100644 src/implementation/200-encoding.md create mode 100644 src/implementation/300-typing.md create mode 100644 src/implementation/400-todos.md create mode 100644 src/implementation/500-function-and-method-ordering.md create mode 100644 src/implementation/600-non-compliant-code.md create mode 100644 src/principles/100-programming-languages-and-frameworks.md create mode 100644 src/principles/200-versions.md create mode 100644 src/principles/300-binaries.md create mode 100644 src/publication/100-publishing.md create mode 100644 src/tests/100-structure.md create mode 100644 src/tests/200-exceptions.md create mode 100644 src/tests/300-fixtures.md create mode 100644 src/tests/400-coverage.md create mode 100644 src/tests/500-static-code-analysis.md diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 5b26fbb..f949216 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -81,6 +81,7 @@ the most critical features are still working after a production deployment. The aim is to ensure that a deployment didn't impact business continuity and detect potential defects immediately after a production release. + ## Programming Languages and Frameworks If we develop in many different programming languages and frameworks, the @@ -108,6 +109,7 @@ encourage exploration and experimentation in a range of programming languages depending on the interest of the individual as part of using Canonical's annual training budget. + ## Charm Ubuntu and Python Version Using inconsistent minor Python version for development, CI and production means @@ -134,6 +136,7 @@ This ensures that the tests are run on the same Python version as the charm will be running in production, catching any issues related to mismatched Python versions. + ## Downloading Binaries Downloading binaries is only permitted from what are classed as trusted sources. @@ -165,6 +168,7 @@ trust (e.g. Launchpad builders or GitHub self-hosted runners). Any exception to this should be specifically noted/documented and approved by IS Charms managers. + ## CI-CD The team maintains a number of repositories which are generally quite similar. @@ -193,6 +197,7 @@ duplication in many repositories. Adding tests to `operator-workflows` will ensure stability of the workflows and provide examples for how to use them. + ## Random Values While creating tests, sometimes you need to assign values to variables @@ -219,12 +224,13 @@ from secrets import token_hex email = token_hex(16) ``` + ## File Encoding If file encoding is not specified when interacting with a file, the default value for the operating sytem is used. The default varies across operating systems reducing the portability of code that does not specify a encoding -explicitly. See: https://peps.python.org/pep-0597/ +explicitly. See: For any file operations, specify the `utf-8` encoding where possible. For example: @@ -241,6 +247,7 @@ Path(...).write_text(..., encoding="utf-8") This ensures that the code we write is portable across operating systems. + ## Repository Setup The repositories that store the source code for our charms are critical to the @@ -270,10 +277,12 @@ and provides access to the repository even if some team members are unavailable. The repository will contain a `CODEOWNERS` file in its root to automatically add the `is-charms` team as reviewer + ``` * @canonical/is-charms ``` + ## PR comments and requests for changes The team uses Github for reviewing changes in the codebase and integrating them @@ -302,6 +311,7 @@ might make it in a different PR or the change might not be desireable in the end. The best approach is having as much feedback from as many engineers as possible to be able to reach a sound decision. + ## Failing Status Checks The team uses GitHub actions to run CI which includes automated status checks @@ -352,6 +362,7 @@ priority so that the team can rely on the automation again. This will ensure that we minimise the number of bugs in our code and tooling. + ## Test Structure Tests that are difficult to understand are of lower value because if they fail @@ -390,6 +401,7 @@ keeping in mind the option of breaking up the test into multiple tests. This structure makes it easy to understand what is required before test execution, how the test works and what it checks for in the end. + ## Test Exception Raised Testing an exception being raised might not be enough as a type of exception @@ -422,6 +434,7 @@ In the above example, the `something` function could have thrown a different `ValueError` related to the argument `b`. Testing the string representation of the `ValueError` ensures the proper type of error is being tested against. + ## Test Fixture When declaring fixture functions and requesting them in the same Python module, @@ -445,6 +458,7 @@ def test_my_fixture(app): assert app == "app" ``` + ## Test Coverage Unit tests check whether the intended functionality has been implemented. This @@ -478,6 +492,7 @@ compared to the main branch as a result of the PR. This ensures a high coverage minimum and no coverage regression. + ## Type Hints Python is a dynamic programming language that does not require type @@ -545,25 +560,28 @@ async def units_fixture(app: ops.model.Application) -> list[ops.model.Unit]: This reduces code duplication which increases the readability of the tests. + ## TODO's in source code Engineers tend to add TODO comments in the source code for different purposes, e.g.: + - To remind themselves to do something before merging a PR. -- To acknowledge that the current state of the code needs improvement, but it +- To acknowledge that the current state of the code needs improvement, but it depends on something external (e.g., a feature not being ready yet). -- To indicate that refactoring is necessary, but the cost is too high +- To indicate that refactoring is necessary, but the cost is too high - (e.g., development efforts or the PR becoming too large). -In general, comments tend to be inaccurate because code changes faster than -comments are updated. The same applies to TODOs. -They tend to stick around forever and confuse future engineers, -who don't know what the TODO is about or if they should take action. -Therefore, TODOs should preferably be added to the product backlog -(e.g., Jira). +In general, comments tend to be inaccurate because code changes faster than +comments are updated. The same applies to TODOs. +They tend to stick around forever and confuse future engineers, +who don't know what the TODO is about or if they should take action. +Therefore, TODOs should preferably be added to the product backlog +(e.g., Jira). It is acceptable to add TODOs while developing a feature on a feature branch, but they should be removed before merging the PR. + ## Static Code Analysis There are many potential problems with code that can be spotted based on @@ -575,23 +593,23 @@ The following automated static code analysis tools should be used locally and enforced through the CI system: - [`black`](https://pypi.org/project/black/) for code formatting - - line length of 99 - - Python target version based on the same is in the + - line length of 99 + - Python target version based on the same is in the [Charm Ubuntu and Python Version](#charm-ubuntu-and-python-version) - [`isort`](https://pypi.org/project/isort/) for import sorting - line length of 99 - `black` profile - [`flake8`](https://pypi.org/project/flake8/) for pythonic code style - - refer to + - refer to [indico `pyproject.toml`](https://github.com/canonical/indico-operator/blob/main/pyproject.toml) - - use the following additional plugins: - - `flake8-docstrings` - - `flake8-docstrings-complete` - - `flake8-test-docs` - - `flake8-copyright` - - `flake8-builtins` - - `pyproject-flake8` - - `pep8-naming` + - use the following additional plugins: + - `flake8-docstrings` + - `flake8-docstrings-complete` + - `flake8-test-docs` + - `flake8-copyright` + - `flake8-builtins` + - `pyproject-flake8` + - `pep8-naming` for additional configurations - [`bandit`](https://pypi.org/project/bandit/) for security checks - [`codespell`](https://pypi.org/project/codespell/) for spelling problems @@ -602,15 +620,16 @@ enforced through the CI system: checks Note: -* Disabling checks should be the last resort, alternatives such as refactoring + +- Disabling checks should be the last resort, alternatives such as refactoring the code should be considered first. For example, instead of disabling the `too-many-arguments` `pylint` rule, consider grouping the arguments, e.g., using a `typing.NamedTuple`. -* When disabling a rule, if the tool allows for it, use the name of the rule +- When disabling a rule, if the tool allows for it, use the name of the rule rather than the code. For example, for `pylint`, always use the name of the rule (like `too-many-arguments`) rather than the code. This makes it easier for readers to know which rule is being disabled and potentially why. -* If a rule is disabled, a comment should to be included above the line +- If a rule is disabled, a comment should to be included above the line disabling the rule explaining why the rule is disabled. This will mean that future readers don't have to guess why it was disabled and can also consider whether the disable can be removed. For rules tied to import, it may be better @@ -626,7 +645,7 @@ Note: import time ``` -* Disabling should be done as specifically as possible. That means, disabling +- Disabling should be done as specifically as possible. That means, disabling the narrowest rule possible on the narrowest section of code. For example, instead of disabling a rule entirely, disable it on a file. Instead of disabling a rule for a file, disable it for just a line of code. The preferred @@ -648,21 +667,21 @@ Note: This ensures consistency across our projects, catches many potential bugs before code is deployed and simplifies PRs. + ## Function and Method Ordering Without a logical order, it can be difficult to follow modules and classes as the number of functions or methods on them grow increasing the maintenance burden of the code. -Functions should be ordered according to the +Functions should be ordered according to the ["step-down"](https://dzone.com/articles/the-stepdown-rule) rule. -This means that a module should be readable from top to bottom, -with functions ordered by level of abstraction, from general to specific. -A calling function should always be above the called function. +This means that a module should be readable from top to bottom, +with functions ordered by level of abstraction, from general to specific. +A calling function should always be above the called function. Functions should also be grouped together logically. If functions have a similar purpose, they should be grouped together. - On classes, the `__init__` method should come first followed by any other factory methods, such as `from_charm`. The rest of the methods on a class should be ordered similar to the guidance for function ordering on modules. @@ -670,6 +689,7 @@ be ordered similar to the guidance for function ordering on modules. This will make it easier for readers to understand the code reducing the maintenance cost. + ## Non Compliant Code Standards and best practices evolve over time which means that code already @@ -692,10 +712,10 @@ This ensures that: as individuals see value in doing so and 4. compliance is enforced using CI where possible. + ## Publishing to a channel -Charms published to tracks different from `latest` should guarantee seemlessly -upgrades between revisions, that is, revisions should not introduce breaking +Charms published to tracks different from `latest` should guarantee seemlessly +upgrades between revisions, that is, revisions should not introduce breaking changes. - diff --git a/CONTRIBUTING.mdt b/CONTRIBUTING.mdt index 57222a2..c6373dc 100644 --- a/CONTRIBUTING.mdt +++ b/CONTRIBUTING.mdt @@ -26,620 +26,41 @@ incorporated by reference. ## Definitions !INCLUDE definitions/*.md -## Programming Languages and Frameworks -If we develop in many different programming languages and frameworks, the -maintenance cost increases, only those with knowledge of a given programming -language or framework can work on code bases in that programming language and -best practices and standards are difficult to set across the team. +!INCLUDE principles/100-programming-languages-and-frameworks.md level=2 -We are a team primarily focused on developing charms, and we should follow the -recommended tools and best practices of the ecosystem we work within (and help -to improve them where appropriate). As a result, the primary programming -language we work in is Python, and the primary framework we use to develop -charms with is the [Operator -Framework](https://github.com/canonical/operator). +!INCLUDE principles/200-versions.md -We will also be working on older charms which may be written in older -frameworks such as the [Reactive -Framework](https://charmsreactive.readthedocs.io/en/latest/). In some cases -we'll work on converting those to the Operator Framework but for small bug -fixes or changes this may not be the case. +!INCLUDE principles/300-binaries.md -As a team we'll also be exposed to a much lesser extent to other programming -languages as a result of working with specific applications that we deploy on -Juju, or via small changes to Juju itself, which is written in Go. We also -encourage exploration and experimentation in a range of programming languages -depending on the interest of the individual as part of using Canonical's -annual training budget. +!INCLUDE ci/100-CI-CD.md -## Charm Ubuntu and Python Version +!INCLUDE implementation/100-random-values.md -Using inconsistent minor Python version for development, CI and production means -that, even if tests pass during development and CI, the charm may not work in -production. +!INCLUDE implementation/200-encoding.md -When charms are running in production, the version of Python available to them -is dictated by the version of Ubuntu that it is running on. This is configured -in the `charmcraft.yaml` file under the `bases.build-on` and `bases.run-on` -keys. The CI should be configured to use the same version of Ubuntu as -configured under the `charmcraft.yaml` `bases.run-on` key. It is recommended -that local development be done on the version of Python that is shipped with -the Ubuntu version defined in `charmcraft.yaml` `bases.run-on`. The unit and -integration tests should be run on the same minor Python version as is shipped -with the the OS as configured under the `charmcraft.yaml` `bases.run-on` key. -With tox, for Ubuntu 22.04, this can be done using: +!INCLUDE github/100-repository-setup.md -``` -[testenv] -basepython = python3.10 -``` +!INCLUDE github/200-prs.md -This ensures that the tests are run on the same Python version as the charm -will be running in production, catching any issues related to mismatched Python -versions. +!INCLUDE ci/200-failing-checks.md -## Downloading Binaries +!INCLUDE tests/100-structure.md -Downloading binaries is only permitted from what are classed as trusted sources. -These are: +!INCLUDE tests/200-exceptions.md -* The Ubuntu Archives (for debian packages) -* Snaps - [owned by the "canonical" account](https://snapcraft.io/publisher/canonical) -* Snaps where the binary is built from a trusted and approved source. -* [PyPi](https://pypi.org/) +!INCLUDE tests/300-fixtures.md -These sources are considered trusted because we are confident that we understand -the way in which they're built, and the security commitments for packages/snaps -produced by them. At any point we can rerun our builds and know that we will -pull in the latest versions of these artefacts, and that in the case of the -first two they will include security updates that Canonical stands behind. +!INCLUDE tests/400-coverage.md -Downloading binaries from any other sources and including them in our charms or -ROCKs exposes our end users to potential security issues either now or in the -future. We don't know the manner in which the binaries were built and don't have -confidence that they will be updated in the future in case of security -vulnerabilities that affect them. +!INCLUDE implementation/300-typing.md -As such, we should ensure that we’re only downloading from either the trusted -sources above, or we're downloading the source instead, verifying that download -where possible, and then building that code from source on infrastructure we -trust (e.g. Launchpad builders or GitHub self-hosted runners). +!INCLUDE implementation/400-todos.md -Any exception to this should be specifically noted/documented and approved by IS -Charms managers. +!INCLUDE tests/500-static-code-analysis.md level=2 -## CI-CD +!INCLUDE implementation/500-function-and-method-ordering.md -The team maintains a number of repositories which are generally quite similar. -If each of the repositories maintains its own workflows, it is difficult to -introduce new tools across the repositories owned by the team. Additionally, if -shared workflows are unreliable, the team is unable to consistently merge -changes and have confidence in the CI-CD. - -Use [`operator-workflows`](https://github.com/canonical/operator-workflows) for -the CI-CD of any repositories owned by the team. Even if the repository is not a -charm, such as a GitHub action, use it to the extent possible. If a repository -has unique requirements for certain workflows, use the `operator-workflows` as -much as possible. Consider proposing changes to `operator-workflows` instead of -doing something custom in a repository. Also consider whether to make partial -use of workflows in `operator-workflows` and add anything that can't be done -with `operator-workflows` to the individual repository, if the CI-CD -requirements are unique to a repository. - -For any changes to `operator-workflows`, add tests just like we expect for any -other repository. - -Using shared workflows will make it easy to evolve the workflows used by all our -repositories and roll out new tooling and checks across them. It will also avoid -duplication in many repositories. - -Adding tests to `operator-workflows` will ensure stability of the workflows and -provide examples for how to use them. - -## Random Values - -While creating tests, sometimes you need to assign values to variables -or parameters in order to simulate a user behavior, for example. In this case, -instead of using constants or fixed values, consider using random ones generated -by [`secrets.token_hex()`](https://docs.python.org/3/library/secrets.html#secrets.token_hex). - -Reasons: - -- If you use the same fixed values in your tests every time, your tests may pass -even if there are underlying issues with your code. This can lead to false -positives and make it difficult to identify and fix real issues in your code. -- Using random values generated by secrets.token_hex() can help to prevent -collisions or conflicts between test data. -- In the case of sensitive data, if you use fixed values in your tests, there is -a risk that may be exposed or leaked, especially if your tests are run in a -shared environment. - -Example: - -```python -from secrets import token_hex - -email = token_hex(16) -``` - -## File Encoding - -If file encoding is not specified when interacting with a file, the default -value for the operating sytem is used. The default varies across operating -systems reducing the portability of code that does not specify a encoding -explicitly. See: https://peps.python.org/pep-0597/ - -For any file operations, specify the `utf-8` encoding where possible. For -example: - -```python -with open(..., encoding="utf-8") as file: - ... - -from pathlib import Path - -Path(...).read_text(encoding="utf-8") -Path(...).write_text(..., encoding="utf-8") -``` - -This ensures that the code we write is portable across operating systems. - -## Repository Setup - -The repositories that store the source code for our charms are critical to the -ongoing development of our charms. They also enforce team policies around code -review and ensure business continuity. If the repository is setup poorly, it -exposes our team and Canonical to operational risks. - -- GitHub should be used for charm source code and issue tracking. -- The repository should be publicly accessible. -- The `is-charms` team is added as maintainers. -- Management and director of the team that owns the charm are added as admins - on the repository. -- Branches are auto-deleted after merging. -- The only option for merging PRs is using a squash commit. -- The default branch is called `main`. -- The default branch is protected and can only be changed using PRs. -- The number of approvers for PRs is 2. -- Approvals are not dismissed on new commits. -- PRs can only be merged if all checks pass. -- Bypassing of the rules is disabled. -- Commits must be signed. -- [Automated secret scanning](https://docs.github.com/en/code-security/secret-scanning/configuring-secret-scanning-for-your-repositories#enabling-secret-scanning-alerts-for-users) - must be enabled. - -The above configuration ensures our team processes around changes are enforced -and provides access to the repository even if some team members are unavailable. - -The repository will contain a `CODEOWNERS` file in its root to automatically add -the `is-charms` team as reviewer -``` -* @canonical/is-charms -``` - -## PR comments and requests for changes - -The team uses Github for reviewing changes in the codebase and integrating them -within our projects. A reviewer can comment and give feedback that potentially -leads to new changes in the submitted code. Github allows you to `request -changes` on a PR, meaning it cannot be merged until the changes are accepted. As -the team is distributed, requesting changes as a default slows down merges and -requires the reviewer to approve again. Even with sufficient approvals, a PR -cannot be merged unless the requestor accepts the changes, which might be -trivial. - -Our team favors commenting in the usual case and resorting to requesting changes -only if there is something problematic. It is adviseable to comment and let the -engineer take action rather than request changes and block the PR. The preferred -way is to comment instead. Note that committing new changes will reset -approvals either way and approvals need to be collected again, which is the -expected behavior and part of our workflow. Please note we expect the engineer -that raised the PR to deal with the comments in good faith, i.e., not just mark -them as resolved when they are not really resolved for the purpose of being able -to merge the PR. The engineer that raised the PR is responsible for closing the -comments once they are tackled. - -With commenting rather than requesting changes, an engineer gives feedback and -still allows the team (based on approvals) to decide the way forward - a change -might make it in a different PR or the change might not be desireable in the -end. The best approach is having as much feedback from as many engineers as -possible to be able to reach a sound decision. - -## Failing Status Checks - -The team uses GitHub actions to run CI which includes automated status checks -to verify the code works as expected. If PRs are merged with failing status -checks, there are potential bugs in the code which may lead to downtime or -other operational issues or a shared tool not working as expected. - -PRs can only be merged if all status checks pass. Some exceptions could -include: - -* If the team has recently taken over a new charm and changes are urgently - needed and the test suit of the existing charm is failing for spurious - reasons. -* A tool we depend on is not working, no previous working version is available - and a change is urgently needed to fix a critical issue in production. - -Even in the above cases it is not clear whether it is reasonable to merge a PR -with failing checks due to the high risks of ignoring failing checks that have -been adopted by the team. Judgement is required in these cases weighing the -risks of introducing bugs with the urgency and impact of the underlying need to -land the change. - -Alternatives to merging the PR with failing status checks include: - -* Change the code to fix the problem. -* Disable the status check (e.g., mark the test as - [`xfail`](https://docs.pytest.org/en/7.1.x/how-to/skipping.html)). This - should not be done lightly as the value the status check provides to the team - is lost. -* Wait for an upstream fix for the issue. - -If it is deemed that a change should land despite a failing status check, the -following artifacts should be added to the PR: - -* If the status check can be run in another way, e.g. locally, a copy of the run - (e.g., a copy of the terminal output) including the git commit SHA which is - being approved and the status check passing -* The reason why the PR needs to be merged without the status check passing - (e.g., because the fix is needed in production and the status check is failing - due to problems with GitHub). - -One of the repository admins should then be asked to review the artifacts and -merge the PR after completing the review. The PR should be otherwise ready to be -merged (e.g., has been approved). - -Resolving the underlying reason the status check is failing should be high -priority so that the team can rely on the automation again. - -This will ensure that we minimise the number of bugs in our code and tooling. - -## Test Structure - -Tests that are difficult to understand are of lower value because if they fail -it is difficult to understand why they fail. - -The docstring of a test has 3 sections, *arrange*, *act* and *assert*. Arrange -explains the pre-conditions required for the test, act explains what steps the -test performs and assert explains what the state must be after all actions are -complete. If the description for a section is longer than one line, any -additional lines are indented by the default indentation of the file. The test -code is separated into blocks for each section. For example: - -```Python -def test_something(): - """ - arrange: given 2 numbers - act: when they are added - assert: the result must be the sum of the 2 numbers. If a description spans - multiple lines, lines after the first should be indented by the default - spacing of the file to indicate that the description continues. - """ - num_a = 1 - num_b = 2 - - result = num_a + num_b - - assert result == 3 -``` - -Whilst this standard is usually valuable, there are cases where it imposes -unreasonable constraints. An example is complex functional tests that check -multiple interactions that require individual arrange, act, assert blocks -because they, for example, build on each other. In those cases, apply judgement -keeping in mind the option of breaking up the test into multiple tests. - -This structure makes it easy to understand what is required before test -execution, how the test works and what it checks for in the end. - -## Test Exception Raised - -Testing an exception being raised might not be enough as a type of exception -can be raised due to multiple reasons. E.g., a function could raise `ValueError` -for different arguments. - -The property of the exception being raised should be asserted against as well. -A common test would be the string representation of the exception. Using the -following `pytest` example: - -```Python -def something(a: int, b: bool): - """Do something.""" - if a >= 5: - raise ValueError("Argument a must be less than 5") - if b: - raise ValueError("Some other error message") - # Rest of the function... - - -def test_something(): - """Test the something function.""" - with pytest.raises(ValueError) as err: - something(a=10, b=False) - assert "Argument a" in str(err.value) - assert "less than 5" in str(err.value) -``` - -In the above example, the `something` function could have thrown a different -`ValueError` related to the argument `b`. Testing the string representation of -the `ValueError` ensures the proper type of error is being tested against. - -## Test Fixture - -When declaring fixture functions and requesting them in the same Python module, -there is a risk of accidentally using the fixture function declared in the -global scope instead of requesting it in the test case function parameters. - -To prevent this problem, it is recommended to add a "fixture" prefix or suffix -to the fixture function name and use the `name` argument in the `pytest.fixture` -decorator to name the fixture. - -Here's an example of using the `name` argument in `pytest.fixture`: - -```python -import pytest - -@pytest.fixture(name="app") -def app_fixture(): - return "app" - -def test_my_fixture(app): - assert app == "app" -``` - -## Test Coverage - -Unit tests check whether the intended functionality has been implemented. This -is valuable to reduce bugs and checking whether any changes to the code break -any features. The benefits of testing are roughly proportional to the test -coverage percentage, the lower the coverage the higher the chances of bugs and -the higher the risk of code changes. - -The team has a coverage percentage which is the maximum of 85% and the current -percentage on the default branch, usually `main`. Any code that is already -covered on the default branch should not cease to be covered by new commits to -main, such as through a pull request. Any coverage exclusion should have an -explanatory comment, such as: - -```Python -# Exclude from coverage since unit tests should not run as __main__ -if __name__ == "__main__": # pragma: no cover - ... -``` - -To enforce this, the `pyproject.toml` file should include the following -configuration: - -```toml -[tool.coverage.report] -fail_under = -``` - -This value should be updated in each PR to reflect any increase in coverage -compared to the main branch as a result of the PR. - -This ensures a high coverage minimum and no coverage regression. - -## Type Hints - -Python is a dynamic programming language that does not require type -declarations. Without type information, arguments might be passed to functions -that are not of the expected type (e.g., passing `None` where it is not -expected), which leads to more bugs. It also makes it more difficult to know -what functions accept as input and return as output. - -Except when impractical, declare type hints on function parameters, return -values and class and instance variables. Examples of when type hints might -be impractical (not an exhaustive list): - -- dictionaries with many nested dictionaries, -- decorator functions, -- when making small changes or -- contributions to projects not owned by the team. - -To leverage the power of type hints, the following configuration snippet should -be added to `pyproject.toml`. This helps the user during the linting process by -ensuring that all functions, including tests, have type definitions and checks -for any typing issues even if a function does not have explicit type hints on -it. - -```toml -[tool.mypy] -check_untyped_defs = true -disallow_untyped_defs = true - -[[tool.mypy.overrides]] -module = "tests.*" -disallow_untyped_defs = false -``` - -The type hints should be checked with `mypy`. More information on -type hints can be found here: [PEP 484](https://peps.python.org/pep-0484/). - -This will help users know what functions expect as parameters and return and -catches more bugs earlier. - -## Handling Typing Issues with python-libjuju - -In tests and elsewhere when interacting with `python-libjuju`, it is a frequent -requirement to check whether certain attributes are `None`. Doing this in many -tests reduces the readability of the code. - -Instead of putting `assert ops_test.model` in individual tests, write a fixture: - -```Python -@pytest_asyncio.fixture(scope="module", name="model") -async def model_fixture(ops_test: pytest_operator.plugin.OpsTest) -> ops.model.Model: - """The current test model.""" - assert ops_test.model - return ops_test.model -``` - -Instead of putting `assert hasattr(app, "units")`, write a fixture: - -```Python -@pytest_asyncio.fixture(scope="function", name="units") -async def units_fixture(app: ops.model.Application) -> list[ops.model.Unit]: - """The current test unit.""" - assert hasattr(app, "units") - return app.units -``` - -This reduces code duplication which increases the readability of the tests. - -## TODO's in source code - -Engineers tend to add TODO comments in the source code for different purposes, -e.g.: -- To remind themselves to do something before merging a PR. -- To acknowledge that the current state of the code needs improvement, but it -depends on something external (e.g., a feature not being ready yet). -- To indicate that refactoring is necessary, but the cost is too high -- (e.g., development efforts or the PR becoming too large). - -In general, comments tend to be inaccurate because code changes faster than -comments are updated. The same applies to TODOs. -They tend to stick around forever and confuse future engineers, -who don't know what the TODO is about or if they should take action. -Therefore, TODOs should preferably be added to the product backlog -(e.g., Jira). -It is acceptable to add TODOs while developing a feature on a feature branch, -but they should be removed before merging the PR. - -## Static Code Analysis - -There are many potential problems with code that can be spotted based on -analysing source code without executing it, such as mismatches in type -expectations. Additionally, code formatting discussions during PRs can be -cumbersome and take up a lot of time. - -The following automated static code analysis tools should be used locally and -enforced through the CI system: - -- [`black`](https://pypi.org/project/black/) for code formatting - - line length of 99 - - Python target version based on the same is in the - [Charm Ubuntu and Python Version](#charm-ubuntu-and-python-version) -- [`isort`](https://pypi.org/project/isort/) for import sorting - - line length of 99 - - `black` profile -- [`flake8`](https://pypi.org/project/flake8/) for pythonic code style - - refer to - [indico `pyproject.toml`](https://github.com/canonical/indico-operator/blob/main/pyproject.toml) - - use the following additional plugins: - - `flake8-docstrings` - - `flake8-docstrings-complete` - - `flake8-test-docs` - - `flake8-copyright` - - `flake8-builtins` - - `pyproject-flake8` - - `pep8-naming` - for additional configurations -- [`bandit`](https://pypi.org/project/bandit/) for security checks -- [`codespell`](https://pypi.org/project/codespell/) for spelling problems -- [`woke`](https://snapcraft.io/woke) for inclusive language -- [`prettier`](https://prettier.io) for JSON and YAML formatting -- [`mypy`](https://pypi.org/project/mypy/) for type checks -- [`pylint`](https://pypi.org/project/pylint/) for further python code style - checks - -Note: -* Disabling checks should be the last resort, alternatives such as refactoring - the code should be considered first. For example, instead of disabling the - `too-many-arguments` `pylint` rule, consider grouping the arguments, e.g., - using a `typing.NamedTuple`. -* When disabling a rule, if the tool allows for it, use the name of the rule - rather than the code. For example, for `pylint`, always use the name of the - rule (like `too-many-arguments`) rather than the code. This makes it easier - for readers to know which rule is being disabled and potentially why. -* If a rule is disabled, a comment should to be included above the line - disabling the rule explaining why the rule is disabled. This will mean that - future readers don't have to guess why it was disabled and can also consider - whether the disable can be removed. For rules tied to import, it may be better - to comment on each usage rather than on the import. Note that comments for - import must be added at the start of an import section to prevent the - formatter from messing up the import sections: - - ``` - # Comment explaining why subprocess is imported. - import logging - import os - import subprocess # nosec B404 - import time - ``` - -* Disabling should be done as specifically as possible. That means, disabling - the narrowest rule possible on the narrowest section of code. For example, - instead of disabling a rule entirely, disable it on a file. Instead of - disabling a rule for a file, disable it for just a line of code. The preferred - way is to disable on a line of code: - - ``` - # disable rule - ``` - - If a rule needs to be disabled for a section, re-enable it as soon as - possible: - - ``` - # disable rule - - # enable rule - ``` - -This ensures consistency across our projects, catches many potential bugs -before code is deployed and simplifies PRs. - -## Function and Method Ordering - -Without a logical order, it can be difficult to follow modules and classes as -the number of functions or methods on them grow increasing the maintenance -burden of the code. - -Functions should be ordered according to the -["step-down"](https://dzone.com/articles/the-stepdown-rule) rule. -This means that a module should be readable from top to bottom, -with functions ordered by level of abstraction, from general to specific. -A calling function should always be above the called function. -Functions should also be grouped together logically. If functions have a -similar purpose, they should be grouped together. - - -On classes, the `__init__` method should come first followed by any other -factory methods, such as `from_charm`. The rest of the methods on a class should -be ordered similar to the guidance for function ordering on modules. - -This will make it easier for readers to understand the code reducing the -maintenance cost. - -## Non Compliant Code - -Standards and best practices evolve over time which means that code already -written may not comply with a new standard. If this is wide spread, it can lead -to a culture of ignoring the standard and can degreade the value of team -standards. - -At a minimum, any code changed in a pull request complies with team standards. -The team values initiative updating code to comply with new standards and -recognises that this needs to be balanced with other priorities, such as -delivering new features of fixing bugs. If possible, standards should be -enforced automatically by the CI system. - -This ensures that: - -1. code under active development is likely to be compliant with team standards, -2. less time is spent updating code that doesn't need to be updated frequently - to new standards, -3. it encourages the team to go out of their way to implement any new standards - as individuals see value in doing so and -4. compliance is enforced using CI where possible. - -## Publishing to a channel - -Charms published to tracks different from `latest` should guarantee seemlessly -upgrades between revisions, that is, revisions should not introduce breaking -changes. +!INCLUDE implementation/600-non-compliant-code.md +!INCLUDE publication/100-publishing.md \ No newline at end of file diff --git a/scripts/render.py b/scripts/render.py index ab0c229..1868301 100644 --- a/scripts/render.py +++ b/scripts/render.py @@ -5,6 +5,7 @@ content = Path("CONTRIBUTING.mdt") heading_level = 0 +in_comment = False for line in content.read_text().split("\n"): if line.startswith("#"): heading_level = len(line.split(" ")[0]) @@ -13,9 +14,16 @@ print(line) continue - include_pattern = line.split(" ")[1] + params = line.split(" ") + include_pattern = params[1] + if len(params) > 2 and params[2].startswith("level="): + heading_level = int(params[2][6:]) - 1 + for included_file in sorted(Path(".").rglob(include_pattern)): for iline in included_file.read_text().split("\n"): - if iline.startswith("#"): + if iline.startswith("```"): + in_comment = not in_comment + + if iline.startswith("#") and not in_comment: iline = "#" * heading_level + iline print(iline) diff --git a/src/ci/100-CI-CD.md b/src/ci/100-CI-CD.md new file mode 100644 index 0000000..522d9da --- /dev/null +++ b/src/ci/100-CI-CD.md @@ -0,0 +1,27 @@ +# CI-CD + +The team maintains a number of repositories which are generally quite similar. +If each of the repositories maintains its own workflows, it is difficult to +introduce new tools across the repositories owned by the team. Additionally, if +shared workflows are unreliable, the team is unable to consistently merge +changes and have confidence in the CI-CD. + +Use [`operator-workflows`](https://github.com/canonical/operator-workflows) for +the CI-CD of any repositories owned by the team. Even if the repository is not a +charm, such as a GitHub action, use it to the extent possible. If a repository +has unique requirements for certain workflows, use the `operator-workflows` as +much as possible. Consider proposing changes to `operator-workflows` instead of +doing something custom in a repository. Also consider whether to make partial +use of workflows in `operator-workflows` and add anything that can't be done +with `operator-workflows` to the individual repository, if the CI-CD +requirements are unique to a repository. + +For any changes to `operator-workflows`, add tests just like we expect for any +other repository. + +Using shared workflows will make it easy to evolve the workflows used by all our +repositories and roll out new tooling and checks across them. It will also avoid +duplication in many repositories. + +Adding tests to `operator-workflows` will ensure stability of the workflows and +provide examples for how to use them. diff --git a/src/ci/200-failing-checks.md b/src/ci/200-failing-checks.md new file mode 100644 index 0000000..156e205 --- /dev/null +++ b/src/ci/200-failing-checks.md @@ -0,0 +1,49 @@ +# Failing Status Checks + +The team uses GitHub actions to run CI which includes automated status checks +to verify the code works as expected. If PRs are merged with failing status +checks, there are potential bugs in the code which may lead to downtime or +other operational issues or a shared tool not working as expected. + +PRs can only be merged if all status checks pass. Some exceptions could +include: + +* If the team has recently taken over a new charm and changes are urgently + needed and the test suit of the existing charm is failing for spurious + reasons. +* A tool we depend on is not working, no previous working version is available + and a change is urgently needed to fix a critical issue in production. + +Even in the above cases it is not clear whether it is reasonable to merge a PR +with failing checks due to the high risks of ignoring failing checks that have +been adopted by the team. Judgement is required in these cases weighing the +risks of introducing bugs with the urgency and impact of the underlying need to +land the change. + +Alternatives to merging the PR with failing status checks include: + +* Change the code to fix the problem. +* Disable the status check (e.g., mark the test as + [`xfail`](https://docs.pytest.org/en/7.1.x/how-to/skipping.html)). This + should not be done lightly as the value the status check provides to the team + is lost. +* Wait for an upstream fix for the issue. + +If it is deemed that a change should land despite a failing status check, the +following artifacts should be added to the PR: + +* If the status check can be run in another way, e.g. locally, a copy of the run + (e.g., a copy of the terminal output) including the git commit SHA which is + being approved and the status check passing +* The reason why the PR needs to be merged without the status check passing + (e.g., because the fix is needed in production and the status check is failing + due to problems with GitHub). + +One of the repository admins should then be asked to review the artifacts and +merge the PR after completing the review. The PR should be otherwise ready to be +merged (e.g., has been approved). + +Resolving the underlying reason the status check is failing should be high +priority so that the team can rely on the automation again. + +This will ensure that we minimise the number of bugs in our code and tooling. diff --git a/src/github/100-repository-setup.md b/src/github/100-repository-setup.md new file mode 100644 index 0000000..99e2d47 --- /dev/null +++ b/src/github/100-repository-setup.md @@ -0,0 +1,33 @@ +# Repository Setup + +The repositories that store the source code for our charms are critical to the +ongoing development of our charms. They also enforce team policies around code +review and ensure business continuity. If the repository is setup poorly, it +exposes our team and Canonical to operational risks. + +- GitHub should be used for charm source code and issue tracking. +- The repository should be publicly accessible. +- The `is-charms` team is added as maintainers. +- Management and director of the team that owns the charm are added as admins + on the repository. +- Branches are auto-deleted after merging. +- The only option for merging PRs is using a squash commit. +- The default branch is called `main`. +- The default branch is protected and can only be changed using PRs. +- The number of approvers for PRs is 2. +- Approvals are not dismissed on new commits. +- PRs can only be merged if all checks pass. +- Bypassing of the rules is disabled. +- Commits must be signed. +- [Automated secret scanning](https://docs.github.com/en/code-security/secret-scanning/configuring-secret-scanning-for-your-repositories#enabling-secret-scanning-alerts-for-users) + must be enabled. + +The above configuration ensures our team processes around changes are enforced +and provides access to the repository even if some team members are unavailable. + +The repository will contain a `CODEOWNERS` file in its root to automatically add +the `is-charms` team as reviewer + +``` +* @canonical/is-charms +``` diff --git a/src/github/200-prs.md b/src/github/200-prs.md new file mode 100644 index 0000000..6cbeded --- /dev/null +++ b/src/github/200-prs.md @@ -0,0 +1,27 @@ +# PR comments and requests for changes + +The team uses Github for reviewing changes in the codebase and integrating them +within our projects. A reviewer can comment and give feedback that potentially +leads to new changes in the submitted code. Github allows you to `request +changes` on a PR, meaning it cannot be merged until the changes are accepted. As +the team is distributed, requesting changes as a default slows down merges and +requires the reviewer to approve again. Even with sufficient approvals, a PR +cannot be merged unless the requestor accepts the changes, which might be +trivial. + +Our team favors commenting in the usual case and resorting to requesting changes +only if there is something problematic. It is adviseable to comment and let the +engineer take action rather than request changes and block the PR. The preferred +way is to comment instead. Note that committing new changes will reset +approvals either way and approvals need to be collected again, which is the +expected behavior and part of our workflow. Please note we expect the engineer +that raised the PR to deal with the comments in good faith, i.e., not just mark +them as resolved when they are not really resolved for the purpose of being able +to merge the PR. The engineer that raised the PR is responsible for closing the +comments once they are tackled. + +With commenting rather than requesting changes, an engineer gives feedback and +still allows the team (based on approvals) to decide the way forward - a change +might make it in a different PR or the change might not be desireable in the +end. The best approach is having as much feedback from as many engineers as +possible to be able to reach a sound decision. diff --git a/src/implementation/100-random-values.md b/src/implementation/100-random-values.md new file mode 100644 index 0000000..f48afd2 --- /dev/null +++ b/src/implementation/100-random-values.md @@ -0,0 +1,25 @@ +# Random Values + +While creating tests, sometimes you need to assign values to variables +or parameters in order to simulate a user behavior, for example. In this case, +instead of using constants or fixed values, consider using random ones generated +by [`secrets.token_hex()`](https://docs.python.org/3/library/secrets.html#secrets.token_hex). + +Reasons: + +- If you use the same fixed values in your tests every time, your tests may pass +even if there are underlying issues with your code. This can lead to false +positives and make it difficult to identify and fix real issues in your code. +- Using random values generated by secrets.token_hex() can help to prevent +collisions or conflicts between test data. +- In the case of sensitive data, if you use fixed values in your tests, there is +a risk that may be exposed or leaked, especially if your tests are run in a +shared environment. + +Example: + +```python +from secrets import token_hex + +email = token_hex(16) +``` diff --git a/src/implementation/200-encoding.md b/src/implementation/200-encoding.md new file mode 100644 index 0000000..f3daee4 --- /dev/null +++ b/src/implementation/200-encoding.md @@ -0,0 +1,21 @@ +# File Encoding + +If file encoding is not specified when interacting with a file, the default +value for the operating sytem is used. The default varies across operating +systems reducing the portability of code that does not specify a encoding +explicitly. See: + +For any file operations, specify the `utf-8` encoding where possible. For +example: + +```python +with open(..., encoding="utf-8") as file: + ... + +from pathlib import Path + +Path(...).read_text(encoding="utf-8") +Path(...).write_text(..., encoding="utf-8") +``` + +This ensures that the code we write is portable across operating systems. diff --git a/src/implementation/300-typing.md b/src/implementation/300-typing.md new file mode 100644 index 0000000..0a303bd --- /dev/null +++ b/src/implementation/300-typing.md @@ -0,0 +1,66 @@ +# Type Hints + +Python is a dynamic programming language that does not require type +declarations. Without type information, arguments might be passed to functions +that are not of the expected type (e.g., passing `None` where it is not +expected), which leads to more bugs. It also makes it more difficult to know +what functions accept as input and return as output. + +Except when impractical, declare type hints on function parameters, return +values and class and instance variables. Examples of when type hints might +be impractical (not an exhaustive list): + +- dictionaries with many nested dictionaries, +- decorator functions, +- when making small changes or +- contributions to projects not owned by the team. + +To leverage the power of type hints, the following configuration snippet should +be added to `pyproject.toml`. This helps the user during the linting process by +ensuring that all functions, including tests, have type definitions and checks +for any typing issues even if a function does not have explicit type hints on +it. + +```toml +[tool.mypy] +check_untyped_defs = true +disallow_untyped_defs = true + +[[tool.mypy.overrides]] +module = "tests.*" +disallow_untyped_defs = false +``` + +The type hints should be checked with `mypy`. More information on +type hints can be found here: [PEP 484](https://peps.python.org/pep-0484/). + +This will help users know what functions expect as parameters and return and +catches more bugs earlier. + +# Handling Typing Issues with python-libjuju + +In tests and elsewhere when interacting with `python-libjuju`, it is a frequent +requirement to check whether certain attributes are `None`. Doing this in many +tests reduces the readability of the code. + +Instead of putting `assert ops_test.model` in individual tests, write a fixture: + +```Python +@pytest_asyncio.fixture(scope="module", name="model") +async def model_fixture(ops_test: pytest_operator.plugin.OpsTest) -> ops.model.Model: + """The current test model.""" + assert ops_test.model + return ops_test.model +``` + +Instead of putting `assert hasattr(app, "units")`, write a fixture: + +```Python +@pytest_asyncio.fixture(scope="function", name="units") +async def units_fixture(app: ops.model.Application) -> list[ops.model.Unit]: + """The current test unit.""" + assert hasattr(app, "units") + return app.units +``` + +This reduces code duplication which increases the readability of the tests. diff --git a/src/implementation/400-todos.md b/src/implementation/400-todos.md new file mode 100644 index 0000000..969e657 --- /dev/null +++ b/src/implementation/400-todos.md @@ -0,0 +1,19 @@ +# TODO's in source code + +Engineers tend to add TODO comments in the source code for different purposes, +e.g.: + +- To remind themselves to do something before merging a PR. +- To acknowledge that the current state of the code needs improvement, but it +depends on something external (e.g., a feature not being ready yet). +- To indicate that refactoring is necessary, but the cost is too high +- (e.g., development efforts or the PR becoming too large). + +In general, comments tend to be inaccurate because code changes faster than +comments are updated. The same applies to TODOs. +They tend to stick around forever and confuse future engineers, +who don't know what the TODO is about or if they should take action. +Therefore, TODOs should preferably be added to the product backlog +(e.g., Jira). +It is acceptable to add TODOs while developing a feature on a feature branch, +but they should be removed before merging the PR. diff --git a/src/implementation/500-function-and-method-ordering.md b/src/implementation/500-function-and-method-ordering.md new file mode 100644 index 0000000..70463bf --- /dev/null +++ b/src/implementation/500-function-and-method-ordering.md @@ -0,0 +1,20 @@ +# Function and Method Ordering + +Without a logical order, it can be difficult to follow modules and classes as +the number of functions or methods on them grow increasing the maintenance +burden of the code. + +Functions should be ordered according to the +["step-down"](https://dzone.com/articles/the-stepdown-rule) rule. +This means that a module should be readable from top to bottom, +with functions ordered by level of abstraction, from general to specific. +A calling function should always be above the called function. +Functions should also be grouped together logically. If functions have a +similar purpose, they should be grouped together. + +On classes, the `__init__` method should come first followed by any other +factory methods, such as `from_charm`. The rest of the methods on a class should +be ordered similar to the guidance for function ordering on modules. + +This will make it easier for readers to understand the code reducing the +maintenance cost. diff --git a/src/implementation/600-non-compliant-code.md b/src/implementation/600-non-compliant-code.md new file mode 100644 index 0000000..ff5491d --- /dev/null +++ b/src/implementation/600-non-compliant-code.md @@ -0,0 +1,21 @@ +# Non Compliant Code + +Standards and best practices evolve over time which means that code already +written may not comply with a new standard. If this is wide spread, it can lead +to a culture of ignoring the standard and can degreade the value of team +standards. + +At a minimum, any code changed in a pull request complies with team standards. +The team values initiative updating code to comply with new standards and +recognises that this needs to be balanced with other priorities, such as +delivering new features of fixing bugs. If possible, standards should be +enforced automatically by the CI system. + +This ensures that: + +1. code under active development is likely to be compliant with team standards, +2. less time is spent updating code that doesn't need to be updated frequently + to new standards, +3. it encourages the team to go out of their way to implement any new standards + as individuals see value in doing so and +4. compliance is enforced using CI where possible. diff --git a/src/principles/100-programming-languages-and-frameworks.md b/src/principles/100-programming-languages-and-frameworks.md new file mode 100644 index 0000000..23de0ca --- /dev/null +++ b/src/principles/100-programming-languages-and-frameworks.md @@ -0,0 +1,26 @@ +# Programming Languages and Frameworks + +If we develop in many different programming languages and frameworks, the +maintenance cost increases, only those with knowledge of a given programming +language or framework can work on code bases in that programming language and +best practices and standards are difficult to set across the team. + +We are a team primarily focused on developing charms, and we should follow the +recommended tools and best practices of the ecosystem we work within (and help +to improve them where appropriate). As a result, the primary programming +language we work in is Python, and the primary framework we use to develop +charms with is the [Operator +Framework](https://github.com/canonical/operator). + +We will also be working on older charms which may be written in older +frameworks such as the [Reactive +Framework](https://charmsreactive.readthedocs.io/en/latest/). In some cases +we'll work on converting those to the Operator Framework but for small bug +fixes or changes this may not be the case. + +As a team we'll also be exposed to a much lesser extent to other programming +languages as a result of working with specific applications that we deploy on +Juju, or via small changes to Juju itself, which is written in Go. We also +encourage exploration and experimentation in a range of programming languages +depending on the interest of the individual as part of using Canonical's +annual training budget. diff --git a/src/principles/200-versions.md b/src/principles/200-versions.md new file mode 100644 index 0000000..35a88da --- /dev/null +++ b/src/principles/200-versions.md @@ -0,0 +1,25 @@ +# Charm Ubuntu and Python Version + +Using inconsistent minor Python version for development, CI and production means +that, even if tests pass during development and CI, the charm may not work in +production. + +When charms are running in production, the version of Python available to them +is dictated by the version of Ubuntu that it is running on. This is configured +in the `charmcraft.yaml` file under the `bases.build-on` and `bases.run-on` +keys. The CI should be configured to use the same version of Ubuntu as +configured under the `charmcraft.yaml` `bases.run-on` key. It is recommended +that local development be done on the version of Python that is shipped with +the Ubuntu version defined in `charmcraft.yaml` `bases.run-on`. The unit and +integration tests should be run on the same minor Python version as is shipped +with the the OS as configured under the `charmcraft.yaml` `bases.run-on` key. +With tox, for Ubuntu 22.04, this can be done using: + +``` +[testenv] +basepython = python3.10 +``` + +This ensures that the tests are run on the same Python version as the charm +will be running in production, catching any issues related to mismatched Python +versions. diff --git a/src/principles/300-binaries.md b/src/principles/300-binaries.md new file mode 100644 index 0000000..cf4f0a5 --- /dev/null +++ b/src/principles/300-binaries.md @@ -0,0 +1,30 @@ +# Downloading Binaries + +Downloading binaries is only permitted from what are classed as trusted sources. +These are: + +* The Ubuntu Archives (for debian packages) +* Snaps + [owned by the "canonical" account](https://snapcraft.io/publisher/canonical) +* Snaps where the binary is built from a trusted and approved source. +* [PyPi](https://pypi.org/) + +These sources are considered trusted because we are confident that we understand +the way in which they're built, and the security commitments for packages/snaps +produced by them. At any point we can rerun our builds and know that we will +pull in the latest versions of these artefacts, and that in the case of the +first two they will include security updates that Canonical stands behind. + +Downloading binaries from any other sources and including them in our charms or +ROCKs exposes our end users to potential security issues either now or in the +future. We don't know the manner in which the binaries were built and don't have +confidence that they will be updated in the future in case of security +vulnerabilities that affect them. + +As such, we should ensure that we’re only downloading from either the trusted +sources above, or we're downloading the source instead, verifying that download +where possible, and then building that code from source on infrastructure we +trust (e.g. Launchpad builders or GitHub self-hosted runners). + +Any exception to this should be specifically noted/documented and approved by IS +Charms managers. diff --git a/src/publication/100-publishing.md b/src/publication/100-publishing.md new file mode 100644 index 0000000..e461242 --- /dev/null +++ b/src/publication/100-publishing.md @@ -0,0 +1,5 @@ +# Publishing to a channel + +Charms published to tracks different from `latest` should guarantee seemlessly +upgrades between revisions, that is, revisions should not introduce breaking +changes. diff --git a/src/tests/100-structure.md b/src/tests/100-structure.md new file mode 100644 index 0000000..1132d58 --- /dev/null +++ b/src/tests/100-structure.md @@ -0,0 +1,37 @@ +# Test Structure + +Tests that are difficult to understand are of lower value because if they fail +it is difficult to understand why they fail. + +The docstring of a test has 3 sections, *arrange*, *act* and *assert*. Arrange +explains the pre-conditions required for the test, act explains what steps the +test performs and assert explains what the state must be after all actions are +complete. If the description for a section is longer than one line, any +additional lines are indented by the default indentation of the file. The test +code is separated into blocks for each section. For example: + +```Python +def test_something(): + """ + arrange: given 2 numbers + act: when they are added + assert: the result must be the sum of the 2 numbers. If a description spans + multiple lines, lines after the first should be indented by the default + spacing of the file to indicate that the description continues. + """ + num_a = 1 + num_b = 2 + + result = num_a + num_b + + assert result == 3 +``` + +Whilst this standard is usually valuable, there are cases where it imposes +unreasonable constraints. An example is complex functional tests that check +multiple interactions that require individual arrange, act, assert blocks +because they, for example, build on each other. In those cases, apply judgement +keeping in mind the option of breaking up the test into multiple tests. + +This structure makes it easy to understand what is required before test +execution, how the test works and what it checks for in the end. diff --git a/src/tests/200-exceptions.md b/src/tests/200-exceptions.md new file mode 100644 index 0000000..a8a892e --- /dev/null +++ b/src/tests/200-exceptions.md @@ -0,0 +1,31 @@ +# Test Exception Raised + +Testing an exception being raised might not be enough as a type of exception +can be raised due to multiple reasons. E.g., a function could raise `ValueError` +for different arguments. + +The property of the exception being raised should be asserted against as well. +A common test would be the string representation of the exception. Using the +following `pytest` example: + +```Python +def something(a: int, b: bool): + """Do something.""" + if a >= 5: + raise ValueError("Argument a must be less than 5") + if b: + raise ValueError("Some other error message") + # Rest of the function... + + +def test_something(): + """Test the something function.""" + with pytest.raises(ValueError) as err: + something(a=10, b=False) + assert "Argument a" in str(err.value) + assert "less than 5" in str(err.value) +``` + +In the above example, the `something` function could have thrown a different +`ValueError` related to the argument `b`. Testing the string representation of +the `ValueError` ensures the proper type of error is being tested against. diff --git a/src/tests/300-fixtures.md b/src/tests/300-fixtures.md new file mode 100644 index 0000000..40caa1a --- /dev/null +++ b/src/tests/300-fixtures.md @@ -0,0 +1,22 @@ +# Test Fixture + +When declaring fixture functions and requesting them in the same Python module, +there is a risk of accidentally using the fixture function declared in the +global scope instead of requesting it in the test case function parameters. + +To prevent this problem, it is recommended to add a "fixture" prefix or suffix +to the fixture function name and use the `name` argument in the `pytest.fixture` +decorator to name the fixture. + +Here's an example of using the `name` argument in `pytest.fixture`: + +```python +import pytest + +@pytest.fixture(name="app") +def app_fixture(): + return "app" + +def test_my_fixture(app): + assert app == "app" +``` diff --git a/src/tests/400-coverage.md b/src/tests/400-coverage.md new file mode 100644 index 0000000..aea65d7 --- /dev/null +++ b/src/tests/400-coverage.md @@ -0,0 +1,32 @@ +# Test Coverage + +Unit tests check whether the intended functionality has been implemented. This +is valuable to reduce bugs and checking whether any changes to the code break +any features. The benefits of testing are roughly proportional to the test +coverage percentage, the lower the coverage the higher the chances of bugs and +the higher the risk of code changes. + +The team has a coverage percentage which is the maximum of 85% and the current +percentage on the default branch, usually `main`. Any code that is already +covered on the default branch should not cease to be covered by new commits to +main, such as through a pull request. Any coverage exclusion should have an +explanatory comment, such as: + +```Python +# Exclude from coverage since unit tests should not run as __main__ +if __name__ == "__main__": # pragma: no cover + ... +``` + +To enforce this, the `pyproject.toml` file should include the following +configuration: + +```toml +[tool.coverage.report] +fail_under = +``` + +This value should be updated in each PR to reflect any increase in coverage +compared to the main branch as a result of the PR. + +This ensures a high coverage minimum and no coverage regression. diff --git a/src/tests/500-static-code-analysis.md b/src/tests/500-static-code-analysis.md new file mode 100644 index 0000000..79d8672 --- /dev/null +++ b/src/tests/500-static-code-analysis.md @@ -0,0 +1,84 @@ +# Static Code Analysis + +There are many potential problems with code that can be spotted based on +analysing source code without executing it, such as mismatches in type +expectations. Additionally, code formatting discussions during PRs can be +cumbersome and take up a lot of time. + +The following automated static code analysis tools should be used locally and +enforced through the CI system: + +- [`black`](https://pypi.org/project/black/) for code formatting + - line length of 99 + - Python target version based on the same is in the + [Charm Ubuntu and Python Version](#charm-ubuntu-and-python-version) +- [`isort`](https://pypi.org/project/isort/) for import sorting + - line length of 99 + - `black` profile +- [`flake8`](https://pypi.org/project/flake8/) for pythonic code style + - refer to + [indico `pyproject.toml`](https://github.com/canonical/indico-operator/blob/main/pyproject.toml) + - use the following additional plugins: + - `flake8-docstrings` + - `flake8-docstrings-complete` + - `flake8-test-docs` + - `flake8-copyright` + - `flake8-builtins` + - `pyproject-flake8` + - `pep8-naming` + for additional configurations +- [`bandit`](https://pypi.org/project/bandit/) for security checks +- [`codespell`](https://pypi.org/project/codespell/) for spelling problems +- [`woke`](https://snapcraft.io/woke) for inclusive language +- [`prettier`](https://prettier.io) for JSON and YAML formatting +- [`mypy`](https://pypi.org/project/mypy/) for type checks +- [`pylint`](https://pypi.org/project/pylint/) for further python code style + checks + +Note: + +- Disabling checks should be the last resort, alternatives such as refactoring + the code should be considered first. For example, instead of disabling the + `too-many-arguments` `pylint` rule, consider grouping the arguments, e.g., + using a `typing.NamedTuple`. +- When disabling a rule, if the tool allows for it, use the name of the rule + rather than the code. For example, for `pylint`, always use the name of the + rule (like `too-many-arguments`) rather than the code. This makes it easier + for readers to know which rule is being disabled and potentially why. +- If a rule is disabled, a comment should to be included above the line + disabling the rule explaining why the rule is disabled. This will mean that + future readers don't have to guess why it was disabled and can also consider + whether the disable can be removed. For rules tied to import, it may be better + to comment on each usage rather than on the import. Note that comments for + import must be added at the start of an import section to prevent the + formatter from messing up the import sections: + + ``` + # Comment explaining why subprocess is imported. + import logging + import os + import subprocess # nosec B404 + import time + ``` + +- Disabling should be done as specifically as possible. That means, disabling + the narrowest rule possible on the narrowest section of code. For example, + instead of disabling a rule entirely, disable it on a file. Instead of + disabling a rule for a file, disable it for just a line of code. The preferred + way is to disable on a line of code: + + ``` + # disable rule + ``` + + If a rule needs to be disabled for a section, re-enable it as soon as + possible: + + ``` + # disable rule + + # enable rule + ``` + +This ensures consistency across our projects, catches many potential bugs +before code is deployed and simplifies PRs. From 236ca82e46bc12841edb86fb4b8dc8ce4703d422 Mon Sep 17 00:00:00 2001 From: Sebastien Georget Date: Tue, 13 Jan 2026 17:39:44 +0100 Subject: [PATCH 4/7] Add comments --- scripts/render.py | 41 ++++++++++++++++++++++++++--------------- 1 file changed, 26 insertions(+), 15 deletions(-) diff --git a/scripts/render.py b/scripts/render.py index 1868301..192dc18 100644 --- a/scripts/render.py +++ b/scripts/render.py @@ -1,29 +1,40 @@ #!/usr/bin/env python3 +""" +Parses a markdown template file (custom schema) +to render a markdown file. + +The markdown template file supports the include of files with this syntax: + !INCLUDE level=xxx + + The script will include the content of "filename" in place adapting the + heading level to xxx if necessary. +""" + from pathlib import Path -content = Path("CONTRIBUTING.mdt") +template = Path("CONTRIBUTING.mdt") heading_level = 0 in_comment = False -for line in content.read_text().split("\n"): - if line.startswith("#"): - heading_level = len(line.split(" ")[0]) +for template_line in template.read_text().split("\n"): + if template_line.startswith("#"): + heading_level = len(template_line.split(" ")[0]) - if not line.startswith("!INCLUDE"): - print(line) + if not template_line.startswith("!INCLUDE"): + print(template_line) continue - params = line.split(" ") - include_pattern = params[1] - if len(params) > 2 and params[2].startswith("level="): - heading_level = int(params[2][6:]) - 1 + include_params = template_line.split(" ") + include_pattern = include_params[1] + if len(include_params) > 2 and include_params[2].startswith("level="): + heading_level = int(include_params[2][6:]) - 1 for included_file in sorted(Path(".").rglob(include_pattern)): - for iline in included_file.read_text().split("\n"): - if iline.startswith("```"): + for included_file_line in included_file.read_text().split("\n"): + if included_file_line.startswith("```"): in_comment = not in_comment - if iline.startswith("#") and not in_comment: - iline = "#" * heading_level + iline - print(iline) + if included_file_line.startswith("#") and not in_comment: + included_file_line = "#" * heading_level + included_file_line + print(included_file_line) From 00713096dc40611b239eb2b77feeec48fb5ee975 Mon Sep 17 00:00:00 2001 From: Sebastien Georget Date: Wed, 14 Jan 2026 17:59:08 +0100 Subject: [PATCH 5/7] Symplify based on review comments --- src/ci/200-failing-checks.md | 8 +++---- src/definitions/300-unit-test.md | 3 +-- src/definitions/500-end-to-end-test.md | 4 ++-- src/definitions/600-smoke-test.md | 4 ++-- src/github/100-repository-setup.md | 8 +++---- src/github/200-prs.md | 13 ++++------- src/tests/500-static-code-analysis.md | 31 ++------------------------ 7 files changed, 19 insertions(+), 52 deletions(-) diff --git a/src/ci/200-failing-checks.md b/src/ci/200-failing-checks.md index 156e205..bc2edd7 100644 --- a/src/ci/200-failing-checks.md +++ b/src/ci/200-failing-checks.md @@ -3,7 +3,7 @@ The team uses GitHub actions to run CI which includes automated status checks to verify the code works as expected. If PRs are merged with failing status checks, there are potential bugs in the code which may lead to downtime or -other operational issues or a shared tool not working as expected. +other operational issues. PRs can only be merged if all status checks pass. Some exceptions could include: @@ -15,8 +15,8 @@ include: and a change is urgently needed to fix a critical issue in production. Even in the above cases it is not clear whether it is reasonable to merge a PR -with failing checks due to the high risks of ignoring failing checks that have -been adopted by the team. Judgement is required in these cases weighing the +with failing checks due to the high risks this entails. +Judgement is required in these cases weighing the risks of introducing bugs with the urgency and impact of the underlying need to land the change. @@ -39,7 +39,7 @@ following artifacts should be added to the PR: (e.g., because the fix is needed in production and the status check is failing due to problems with GitHub). -One of the repository admins should then be asked to review the artifacts and +One of the repository admins should then be asked to review the changes and merge the PR after completing the review. The PR should be otherwise ready to be merged (e.g., has been approved). diff --git a/src/definitions/300-unit-test.md b/src/definitions/300-unit-test.md index 102943f..ebc4af0 100644 --- a/src/definitions/300-unit-test.md +++ b/src/definitions/300-unit-test.md @@ -4,5 +4,4 @@ These are tests that cover charm/ service functions to ensure that given a specific context and mocked interfaces, the function returns the expected output. Tests could cover more than one function if some of them don't include business logic. These tests shouldn't be functional, they just ensure that the -code is doing what it's supposed to do. A test that requires too many mocks -indicates the design needs to be improved to reduce coupling. +code is doing what it's supposed to do. diff --git a/src/definitions/500-end-to-end-test.md b/src/definitions/500-end-to-end-test.md index 00cec60..521b142 100644 --- a/src/definitions/500-end-to-end-test.md +++ b/src/definitions/500-end-to-end-test.md @@ -4,5 +4,5 @@ These are simulated user scenarios running on an environment as close as possible to production. They ususally run on a staging environment with preexisting data, and interact with the charm as a user would (ideally using the juju cli). These are functional tests and ensure that the charm is working as -intended in the condition close to production in order to detect issues related -to this environment (ressources, pre-existing condition, migrations, ...). +intended in the conditions close to production in order to detect issues related +to this environment (ressources, pre-existing conditions, migrations, ...). diff --git a/src/definitions/600-smoke-test.md b/src/definitions/600-smoke-test.md index 8d3b43f..f1f07f1 100644 --- a/src/definitions/600-smoke-test.md +++ b/src/definitions/600-smoke-test.md @@ -1,6 +1,6 @@ # Smoke Tests These are functional tests for business critical use cases used to ensure that -the most critical features are still working after a production deployment. The +the most critical features are still working after a deployment. The aim is to ensure that a deployment didn't impact business continuity and detect -potential defects immediately after a production release. +potential defects immediately after the deployment. diff --git a/src/github/100-repository-setup.md b/src/github/100-repository-setup.md index 99e2d47..d11faef 100644 --- a/src/github/100-repository-setup.md +++ b/src/github/100-repository-setup.md @@ -1,14 +1,14 @@ # Repository Setup The repositories that store the source code for our charms are critical to the -ongoing development of our charms. They also enforce team policies around code -review and ensure business continuity. If the repository is setup poorly, it +ongoing development of our charms. They also enforce quality gates +and ensure business continuity. If the repository is setup poorly, it exposes our team and Canonical to operational risks. - GitHub should be used for charm source code and issue tracking. - The repository should be publicly accessible. -- The `is-charms` team is added as maintainers. -- Management and director of the team that owns the charm are added as admins +- The `platform-engineering` team is added as maintainers. +- `is-devops-leadership` team is added as admins on the repository. - Branches are auto-deleted after merging. - The only option for merging PRs is using a squash commit. diff --git a/src/github/200-prs.md b/src/github/200-prs.md index 6cbeded..1cfaa60 100644 --- a/src/github/200-prs.md +++ b/src/github/200-prs.md @@ -6,15 +6,11 @@ leads to new changes in the submitted code. Github allows you to `request changes` on a PR, meaning it cannot be merged until the changes are accepted. As the team is distributed, requesting changes as a default slows down merges and requires the reviewer to approve again. Even with sufficient approvals, a PR -cannot be merged unless the requestor accepts the changes, which might be -trivial. +cannot be merged unless the requestor accepts the changes. -Our team favors commenting in the usual case and resorting to requesting changes -only if there is something problematic. It is adviseable to comment and let the +It is adviseable to comment and let the engineer take action rather than request changes and block the PR. The preferred -way is to comment instead. Note that committing new changes will reset -approvals either way and approvals need to be collected again, which is the -expected behavior and part of our workflow. Please note we expect the engineer +way is to comment instead. Please note we expect the engineer that raised the PR to deal with the comments in good faith, i.e., not just mark them as resolved when they are not really resolved for the purpose of being able to merge the PR. The engineer that raised the PR is responsible for closing the @@ -23,5 +19,4 @@ comments once they are tackled. With commenting rather than requesting changes, an engineer gives feedback and still allows the team (based on approvals) to decide the way forward - a change might make it in a different PR or the change might not be desireable in the -end. The best approach is having as much feedback from as many engineers as -possible to be able to reach a sound decision. +end. diff --git a/src/tests/500-static-code-analysis.md b/src/tests/500-static-code-analysis.md index 79d8672..91fabb9 100644 --- a/src/tests/500-static-code-analysis.md +++ b/src/tests/500-static-code-analysis.md @@ -5,35 +5,8 @@ analysing source code without executing it, such as mismatches in type expectations. Additionally, code formatting discussions during PRs can be cumbersome and take up a lot of time. -The following automated static code analysis tools should be used locally and -enforced through the CI system: - -- [`black`](https://pypi.org/project/black/) for code formatting - - line length of 99 - - Python target version based on the same is in the - [Charm Ubuntu and Python Version](#charm-ubuntu-and-python-version) -- [`isort`](https://pypi.org/project/isort/) for import sorting - - line length of 99 - - `black` profile -- [`flake8`](https://pypi.org/project/flake8/) for pythonic code style - - refer to - [indico `pyproject.toml`](https://github.com/canonical/indico-operator/blob/main/pyproject.toml) - - use the following additional plugins: - - `flake8-docstrings` - - `flake8-docstrings-complete` - - `flake8-test-docs` - - `flake8-copyright` - - `flake8-builtins` - - `pyproject-flake8` - - `pep8-naming` - for additional configurations -- [`bandit`](https://pypi.org/project/bandit/) for security checks -- [`codespell`](https://pypi.org/project/codespell/) for spelling problems -- [`woke`](https://snapcraft.io/woke) for inclusive language -- [`prettier`](https://prettier.io) for JSON and YAML formatting -- [`mypy`](https://pypi.org/project/mypy/) for type checks -- [`pylint`](https://pypi.org/project/pylint/) for further python code style - checks +The automated static code analysis tools that should be used locally and +enforced through the CI system are listed in the [`lint`](https://github.com/canonical/platform-engineering-charm-template/blob/main/tox.toml#L44) section and the [`static`](https://github.com/canonical/platform-engineering-charm-template/blob/main/tox.toml#L107) section of our charm template. Note: From bb5d8310b0b78f66325c12b712fc945d6193a9ce Mon Sep 17 00:00:00 2001 From: Sebastien Georget Date: Wed, 14 Jan 2026 18:03:32 +0100 Subject: [PATCH 6/7] Lint --- src/github/100-repository-setup.md | 6 +++--- src/tests/500-static-code-analysis.md | 9 +++++---- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/github/100-repository-setup.md b/src/github/100-repository-setup.md index d11faef..3aa1a5a 100644 --- a/src/github/100-repository-setup.md +++ b/src/github/100-repository-setup.md @@ -26,8 +26,8 @@ The above configuration ensures our team processes around changes are enforced and provides access to the repository even if some team members are unavailable. The repository will contain a `CODEOWNERS` file in its root to automatically add -the `is-charms` team as reviewer +the `platform-engineering` team as reviewer -``` -* @canonical/is-charms +```text +* @canonical/platform-engineering ``` diff --git a/src/tests/500-static-code-analysis.md b/src/tests/500-static-code-analysis.md index 91fabb9..dcfb224 100644 --- a/src/tests/500-static-code-analysis.md +++ b/src/tests/500-static-code-analysis.md @@ -6,7 +6,8 @@ expectations. Additionally, code formatting discussions during PRs can be cumbersome and take up a lot of time. The automated static code analysis tools that should be used locally and -enforced through the CI system are listed in the [`lint`](https://github.com/canonical/platform-engineering-charm-template/blob/main/tox.toml#L44) section and the [`static`](https://github.com/canonical/platform-engineering-charm-template/blob/main/tox.toml#L107) section of our charm template. +enforced through the CI system are listed in the [`lint`](https://github.com/canonical/platform-engineering-charm-template/blob/main/tox.toml#L44) +section and the [`static`](https://github.com/canonical/platform-engineering-charm-template/blob/main/tox.toml#L107) section of our charm template. Note: @@ -26,7 +27,7 @@ Note: import must be added at the start of an import section to prevent the formatter from messing up the import sections: - ``` + ```python # Comment explaining why subprocess is imported. import logging import os @@ -40,14 +41,14 @@ Note: disabling a rule for a file, disable it for just a line of code. The preferred way is to disable on a line of code: - ``` + ```python # disable rule ``` If a rule needs to be disabled for a section, re-enable it as soon as possible: - ``` + ```python # disable rule # enable rule From 8f2886d3557ea5cb2318f3391c23c76874ff2e24 Mon Sep 17 00:00:00 2001 From: Sebastien Georget Date: Wed, 14 Jan 2026 18:14:22 +0100 Subject: [PATCH 7/7] Lint --- src/tests/500-static-code-analysis.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/tests/500-static-code-analysis.md b/src/tests/500-static-code-analysis.md index dcfb224..db9c150 100644 --- a/src/tests/500-static-code-analysis.md +++ b/src/tests/500-static-code-analysis.md @@ -6,8 +6,11 @@ expectations. Additionally, code formatting discussions during PRs can be cumbersome and take up a lot of time. The automated static code analysis tools that should be used locally and -enforced through the CI system are listed in the [`lint`](https://github.com/canonical/platform-engineering-charm-template/blob/main/tox.toml#L44) -section and the [`static`](https://github.com/canonical/platform-engineering-charm-template/blob/main/tox.toml#L107) section of our charm template. +enforced through the CI system are listed in the +[`lint`](https://github.com/canonical/platform-engineering-charm-template/blob/main/tox.toml#L44) +section and the +[`static`](https://github.com/canonical/platform-engineering-charm-template/blob/main/tox.toml#L107) +section of our charm template. Note: