Skip to content

Add reviewer guidelines for main providers (infra)#2091

Merged
pieqq merged 9 commits intomainfrom
provider_review_guideline
Aug 29, 2025
Merged

Add reviewer guidelines for main providers (infra)#2091
pieqq merged 9 commits intomainfrom
provider_review_guideline

Conversation

@Hook25
Copy link
Copy Markdown
Collaborator

@Hook25 Hook25 commented Aug 26, 2025

Description

This adds a guideline for reviewing PRs to the main providers in Checkbox. This can be used by authors to avoid common pitfalls when developing Checkbox tests.

Resolved issues

Fixes: https://warthogs.atlassian.net/browse/CHECKBOX-1988

Documentation

This is documentation

Tests

N/A

@Hook25 Hook25 changed the title Add reviewer guidelines for main providers Add reviewer guidelines for main providers (infra) Aug 26, 2025
Copy link
Copy Markdown
Collaborator

@pieqq pieqq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed internally, please add footnotes to justify why certain things need to be done a certain way.

Also, try to avoid using "we". I've made a few suggestions inline, but I didn't cover everything.

Hook25 and others added 5 commits August 27, 2025 17:06
Co-authored-by: Pierre Equoy <pierre.equoy@canonical.com>
Co-authored-by: Pierre Equoy <pierre.equoy@canonical.com>
@pieqq pieqq merged commit 275280b into main Aug 29, 2025
10 checks passed
@pieqq pieqq deleted the provider_review_guideline branch August 29, 2025 07:06
stanley31huang pushed a commit that referenced this pull request Oct 3, 2025
* First draft of README.md

* Fix formatting and add more do/dont

* More rules for python scripts

* Note to use raise SystemExit

* Review feedback 1

Co-authored-by: Pierre Equoy <pierre.equoy@canonical.com>

* Rephrase why no dependencies

Co-authored-by: Pierre Equoy <pierre.equoy@canonical.com>

* Give a rationale for each rule

* Fix links to be interactive

---------

Co-authored-by: Pierre Equoy <pierre.equoy@canonical.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants