Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add code review documentation #6250

Merged
merged 2 commits into from
Sep 14, 2023
Merged

Add code review documentation #6250

merged 2 commits into from
Sep 14, 2023

Conversation

rynowak
Copy link
Contributor

@rynowak rynowak commented Sep 11, 2023

Description

First cut of code review documentation

Type of change

  • This pull request is a minor refactor, code cleanup, test improvement, or other maintenance task and doesn't change the functionality of Radius (issue link optional).

Auto-generated summary

🤖 Generated by Copilot at da98ae1

Summary

📝🔗👀

This pull request adds a new document ./docs/contributing/contributing-code/contributing-code-reviewing/README.md that explains the code review process and best practices for Radius. It also updates the existing documentation on pull requests and how to contribute to include code reviews as a form of participation and quality assurance. The goal of this pull request is to improve the code review culture and standards for the project.

New code-review doc
Helps contributors improve
Quality and feedback

Walkthrough

  • Add a new document on code reviews for Radius ([link](https://github.com/radius-project/radius/pull/6250/files?diff=unified&w=0#diff-197beededa4d4635e30d5c9f2261671669816f2f79a661239c6e719adae5245cR1-R189))

@rynowak rynowak requested a review from a team as a code owner September 11, 2023 18:44
@github-actions
Copy link

github-actions bot commented Sep 11, 2023

Radius functional test overview

🔍 Go to test action run

Name Value
Repository radius-project/radius
Commit ref refs/pull/6250/merge
Unique ID fc1359f1f9
Image tag pr-fc1359f1f9
Click here to see the list of tools in the current test run
  • gotestsum 1.10.0
  • KinD: v0.20.0
  • Dapr: 1.11.0
  • Azure KeyVault CSI driver: 1.4.2
  • Azure Workload identity webhook: 1.1.0
  • Bicep recipe location radiusdev.azurecr.io/test/functional/shared/recipes/<name>:pr-fc1359f1f9
  • Terraform recipe location http://tf-module-server.radius-test-tf-module-server.svc.cluster.local/<name>.zip (in cluster)
  • applications-rp test image location: ghcr.io/radius-project/dev/applications-rp:pr-fc1359f1f9
  • ucp test image location: ghcr.io/radius-project/dev/ucpd:pr-fc1359f1f9
  • deployment-engine test image location: radius.azurecr.io/deployment-engine:latest

Test Status

⌛ Building Radius and pushing container images for functional tests...
✅ Container images build succeeded
⌛ Publishing Bicep Recipes for functional tests...
✅ Recipe publishing succeeded
⌛ Starting datastoresrp functional tests...
⌛ Starting ucp functional tests...
⌛ Starting daprrp functional tests...
⌛ Starting msgrp functional tests...
⌛ Starting shared functional tests...
✅ datastoresrp functional tests succeeded
✅ ucp functional tests succeeded
✅ daprrp functional tests succeeded
✅ samples functional tests succeeded
✅ msgrp functional tests succeeded
✅ shared functional tests succeeded


Since we automate most matters of Style, and try to resolve matters of Behavior and Scope before a pull-request then that means the focus of most reviews should be on the Correctness and Design.

## How to give good feedback
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like more input and feedback from the team on this section. @nicolejms has scheduled a team discussion on the topic of code reviews. I plan to update this again after that meeting.

@github-actions
Copy link

Test Results

2 971 tests  ±0   2 962 ✔️ ±0   2m 1s ⏱️ -12s
   251 suites ±0          9 💤 ±0 
       1 files   ±0          0 ±0 

Results for commit da98ae1. ± Comparison against base commit c0cd383.

This pull request removes 2 and adds 2 tests. Note that renamed tests count towards both.
github.com/radius-project/radius/pkg/armrpc/frontend/controller ‑ TestValidateEtag_IfMatch/961b5ad6-9cde-413b-adba-be7edcdb42fb
github.com/radius-project/radius/pkg/armrpc/frontend/controller ‑ TestValidateEtag_IfMatch/961b5ad6-9cde-413b-adba-be7edcdb42fb#01
github.com/radius-project/radius/pkg/armrpc/frontend/controller ‑ TestValidateEtag_IfMatch/49983c39-6350-4bb8-a1a3-cdee2d3e9ae1
github.com/radius-project/radius/pkg/armrpc/frontend/controller ‑ TestValidateEtag_IfMatch/49983c39-6350-4bb8-a1a3-cdee2d3e9ae1#01

@github-actions
Copy link

66.3

For the detailed report, please go to Checks tab, click Build and Test, and then download unit_test_coverage artifact at the bottom of build page.

  • Your PR branch coverage: 66.3 %
  • main branch coverage: 66.3 %
  • diff coverage: 0 %

The coverage result does not include the functional test coverage.

@rynowak rynowak merged commit 1d75823 into main Sep 14, 2023
1 of 2 checks passed
@rynowak rynowak deleted the rynowak/code-reviews branch September 14, 2023 23:01
@github-actions
Copy link

github-actions bot commented Sep 14, 2023

❌ Container images build failed

❌ Test recipe publishing failed

@github-actions
Copy link

66.3

For the detailed report, please go to Checks tab, click Build and Test, and then download unit_test_coverage artifact at the bottom of build page.

  • Your PR branch coverage: 66.3 %
  • main branch coverage: 66.3 %
  • diff coverage: 0 %

The coverage result does not include the functional test coverage.


We welcome **any contributor or community-member** to engage with any **any pull-request** on our repository as a reviewer.

This page some contains guidance for:
Copy link
Contributor

Choose a reason for hiding this comment

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

"contains some"

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.

2 participants