diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1e86945..1b1753c 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -1,28 +1,16 @@ # 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) +- [Definitions](#definitions) +- [Principles](#principles) +- [Workflows](#workflows) +- [Implementation](#implementation) +- [Tests](#tests) +- [Continuous integration](#continuous-integration) +- [Publication](#publication) The -[How to write and structure charm code](https://documentation.ubuntu.com/ops/latest/howto/write-and-structure-charm-code) -is incorporated by reference. +[Charm development best practices](https://documentation.ubuntu.com/juju/3.6/reference/charm/index.html) +are incorporated by reference. ## Definitions @@ -50,8 +38,7 @@ 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. ### Integration Tests @@ -72,17 +59,19 @@ 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, ...). ### 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. +## Principles + ## Programming Languages and Frameworks If we develop in many different programming languages and frameworks, the @@ -170,36 +159,70 @@ Any exception to this should be specifically noted/documented and approved by IS Charms managers. -## CI-CD +## Workflows -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. +### Repository Setup -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. +The repositories that store the source code for our charms are critical to the +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. -For any changes to `operator-workflows`, add tests just like we expect for any -other repository. +- GitHub should be used for charm source code and issue tracking. +- The repository should be publicly accessible. +- 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. +- 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. -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. +The above configuration ensures our team processes around changes are enforced +and provides access to the repository even if some team members are unavailable. -Adding tests to `operator-workflows` will ensure stability of the workflows and -provide examples for how to use them. +The repository will contain a `CODEOWNERS` file in its root to automatically add +the `platform-engineering` team as reviewer + +```text +* @canonical/platform-engineering +``` -## Random Values +### 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. + +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. 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. + + +## Implementation + +### 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, @@ -226,7 +249,7 @@ email = token_hex(16) ``` -## File Encoding +### 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 @@ -249,122 +272,143 @@ Path(...).write_text(..., encoding="utf-8") This ensures that the code we write is portable across operating systems. -## Repository Setup +### Type Hints -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. +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. -- 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. +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): -The above configuration ensures our team processes around changes are enforced -and provides access to the repository even if some team members are unavailable. +- dictionaries with many nested dictionaries, +- decorator functions, +- when making small changes or +- contributions to projects not owned by the team. -The repository will contain a `CODEOWNERS` file in its root to automatically add -the `is-charms` team as reviewer +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 ``` -* @canonical/is-charms + +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: -## PR comments and requests for changes +```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 +``` -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. +This reduces code duplication which increases the readability of the tests. -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. +### TODO's in source code +Engineers tend to add TODO comments in the source code for different purposes, +e.g.: -## Failing Status Checks +- 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). -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. +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. -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. +### Function and Method Ordering -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. +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. -Alternatives to merging the PR with failing status checks include: +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. -* 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. +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. -If it is deemed that a change should land despite a failing status check, the -following artifacts should be added to the PR: +This will make it easier for readers to understand the code reducing the +maintenance cost. -* 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). +### Non Compliant Code -Resolving the underlying reason the status check is failing should be high -priority so that the team can rely on the automation again. +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. -This will ensure that we minimise the number of bugs in our code and tooling. +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: -## Test Structure +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. + + +## Tests + +### Test Structure Tests that are difficult to understand are of lower value because if they fail it is difficult to understand why they fail. @@ -403,7 +447,7 @@ 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 +### 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` @@ -436,7 +480,7 @@ In the above example, the `something` function could have thrown a different the `ValueError` ensures the proper type of error is being tested against. -## Test Fixture +### 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 @@ -460,7 +504,7 @@ def test_my_fixture(app): ``` -## Test Coverage +### 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 @@ -494,95 +538,6 @@ 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 @@ -590,35 +545,12 @@ 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: @@ -638,7 +570,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 @@ -652,14 +584,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 @@ -669,54 +601,94 @@ This ensures consistency across our projects, catches many potential bugs before code is deployed and simplifies PRs. -## Function and Method Ordering +## Continuous integration -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. +### CI-CD -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. +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. -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. +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. -This will make it easier for readers to understand the code reducing the -maintenance cost. +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. -## Non Compliant Code +Adding tests to `operator-workflows` will ensure stability of the workflows and +provide examples for how to use them. -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. +### Failing Status Checks -This ensures that: +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. -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. +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 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. + +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 changes and +merge the PR after completing the review. The PR should be otherwise ready to be +merged (e.g., has been approved). -## Publishing to a channel +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. + + +## Publication + +### 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/CONTRIBUTING.mdt b/CONTRIBUTING.mdt index c6373dc..bad95a9 100644 --- a/CONTRIBUTING.mdt +++ b/CONTRIBUTING.mdt @@ -1,49 +1,50 @@ # 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](#definitions) +- [Principles](#principles) +- [Workflows](#workflows) +- [Implementation](#implementation) +- [Tests](#tests) +- [Continuous integration](#continuous-integration) +- [Publication](#publication) + +The +[Charm development best practices](https://documentation.ubuntu.com/juju/3.6/reference/charm/index.html) +are incorporated by reference. ## Definitions !INCLUDE definitions/*.md +## Principles + !INCLUDE principles/100-programming-languages-and-frameworks.md level=2 !INCLUDE principles/200-versions.md !INCLUDE principles/300-binaries.md -!INCLUDE ci/100-CI-CD.md +## Workflows + +!INCLUDE github/100-repository-setup.md + +!INCLUDE github/200-prs.md + +## Implementation !INCLUDE implementation/100-random-values.md !INCLUDE implementation/200-encoding.md -!INCLUDE github/100-repository-setup.md +!INCLUDE implementation/300-typing.md -!INCLUDE github/200-prs.md +!INCLUDE implementation/400-todos.md -!INCLUDE ci/200-failing-checks.md +!INCLUDE implementation/500-function-and-method-ordering.md + +!INCLUDE implementation/600-non-compliant-code.md + +## Tests !INCLUDE tests/100-structure.md @@ -53,14 +54,14 @@ incorporated by reference. !INCLUDE tests/400-coverage.md -!INCLUDE implementation/300-typing.md +!INCLUDE tests/500-static-code-analysis.md level=2 -!INCLUDE implementation/400-todos.md +## Continuous integration -!INCLUDE tests/500-static-code-analysis.md level=2 +!INCLUDE ci/100-CI-CD.md -!INCLUDE implementation/500-function-and-method-ordering.md +!INCLUDE ci/200-failing-checks.md -!INCLUDE implementation/600-non-compliant-code.md +## Publication -!INCLUDE publication/100-publishing.md \ No newline at end of file +!INCLUDE publication/100-publishing.md diff --git a/README.md b/README.md index 0e4ce29..7638421 100644 --- a/README.md +++ b/README.md @@ -1,4 +1,4 @@ -# IS Charms Contributing Guide +# Platform Engineering Contributing Guide *This contributing guide is under development.* diff --git a/src/github/100-repository-setup.md b/src/github/100-repository-setup.md index 3aa1a5a..80975db 100644 --- a/src/github/100-repository-setup.md +++ b/src/github/100-repository-setup.md @@ -7,27 +7,9 @@ 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. +- Commits must be signed. - 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. -- 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 `platform-engineering` team as reviewer -```text -* @canonical/platform-engineering -``` +Additional quality gates are applied by the team internal processes.