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 a PR Checklist for Contributions #7921

Merged
merged 4 commits into from
Oct 3, 2024

Conversation

kachawla
Copy link
Contributor

@kachawla kachawla commented Sep 13, 2024

Description

Adding a contribution PR checklist to ensure important areas like, design, samples, and doc updates are covered.

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).

Comment on lines 28 to 33
Please verify that the PR meets the following requirements, where applicable:

- [ ] An overview of proposed schema changes is included in a linked GitHub issue.
- [ ] A design document PR is created in the [design-notes repository](https://github.com/radius-project/design-notes/), if new APIs are being introduced.
- [ ] A PR for the [samples repository](https://github.com/radius-project/samples) is created, if existing samples are affected by the changes in this PR.
- [ ] A PR for the [documentation repository](https://github.com/radius-project/docs) is created, if the changes in this PR affect the documentation.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an initial draft. Please feel free to suggest updates to the list/phrasing etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we talk about tests like adding unit and functional tests?

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 like the idea of adding functional tests in the checklist, but hesitant to add it until we build a consensus with the team on it. Mainly because it existed in the old checklist and was removed due to being ignored. @rynowak, do you have strong opinions on this?

Copy link

codecov bot commented Sep 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.63%. Comparing base (223ec1e) to head (58fd5cf).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7921      +/-   ##
==========================================
- Coverage   60.64%   60.63%   -0.02%     
==========================================
  Files         534      534              
  Lines       28488    28488              
==========================================
- Hits        17277    17273       -4     
- Misses       9698     9700       +2     
- Partials     1513     1515       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kachawla kachawla changed the title Add a PR checklist for pre and post implementation requirements. Add a PR checklist for. Sep 16, 2024
@kachawla kachawla changed the title Add a PR checklist for. Add a PR Checklist for Contributions Sep 16, 2024
Comment on lines +30 to +31
- [ ] An overview of proposed schema changes is included in a linked GitHub issue.
- [ ] A design document PR is created in the [design-notes repository](https://github.com/radius-project/design-notes/), if new APIs are being introduced.
Copy link
Contributor

Choose a reason for hiding this comment

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

should we combine these? they either update the GH issue with the details on the schema changes or they do a design doc if it involves new API changes

Copy link
Contributor Author

@kachawla kachawla Sep 23, 2024

Choose a reason for hiding this comment

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

Could you suggest the user facing phrasing you have on mind for combining these? @Reshrahim


- [ ] An overview of proposed schema changes is included in a linked GitHub issue.
- [ ] A design document PR is created in the [design-notes repository](https://github.com/radius-project/design-notes/), if new APIs are being introduced.
- [ ] A PR for the [samples repository](https://github.com/radius-project/samples) is created, if existing samples are affected by the changes in this PR.
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to include more possibilities so we , say, 'add new samples when new features are introduced'.
One suggestion:
..ensure samples repo adequately addresses changes in this PR (which may include addition/update/deletion) of samples

Copy link
Contributor Author

@kachawla kachawla Sep 23, 2024

Choose a reason for hiding this comment

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

If a new sample needs to be added, then that should be covered in the design review discussion (# 2), we can expand that to new API/significant feature to be explicit, if that helps. This list captures what we agreed on in the team meeting.

Copy link
Contributor

@superbeeny superbeeny left a comment

Choose a reason for hiding this comment

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

Should we include design reviews? Is this template just for creating a PR or should it cover the review and approval process as well. It would give contributors a clear idea of the path to accepting the PR.

@kachawla
Copy link
Contributor Author

Should we include design reviews? Is this template just for creating a PR or should it cover the review and approval process as well. It would give contributors a clear idea of the path to accepting the PR.

Thanks for the review, @superbeeny. Do you mean expanding this checklist item to explicitly to call out review + approval of the design doc?

- [ ] A design document PR is created in the [design-notes repository](https://github.com/radius-project/design-notes/), if new APIs are being introduced.

@superbeeny
Copy link
Contributor

Should we include design reviews? Is this template just for creating a PR or should it cover the review and approval process as well. It would give contributors a clear idea of the path to accepting the PR.

Thanks for the review, @superbeeny. Do you mean expanding this checklist item to explicitly to call out review + approval of the design doc?

- [ ] A design document PR is created in the [design-notes repository](https://github.com/radius-project/design-notes/), if new APIs are being introduced.

Yes, it gives contributors immediate visibility into the status of the PR

@kachawla
Copy link
Contributor Author

Should we include design reviews? Is this template just for creating a PR or should it cover the review and approval process as well. It would give contributors a clear idea of the path to accepting the PR.

Thanks for the review, @superbeeny. Do you mean expanding this checklist item to explicitly to call out review + approval of the design doc?
- [ ] A design document PR is created in the [design-notes repository](https://github.com/radius-project/design-notes/), if new APIs are being introduced.

Yes, it gives contributors immediate visibility into the status of the PR

That makes sense, I'll update it.

@radius-functional-tests
Copy link

radius-functional-tests bot commented Sep 27, 2024

Radius functional test overview

🔍 Go to test action run

Name Value
Repository kachawla/radius
Commit ref 771ac88
Unique ID func6c9ad5ee65
Image tag pr-func6c9ad5ee65
Click here to see the list of tools in the current test run
  • gotestsum 1.12.0
  • KinD: v0.20.0
  • Dapr: 1.12.0
  • Azure KeyVault CSI driver: 1.4.2
  • Azure Workload identity webhook: 1.3.0
  • Bicep recipe location ghcr.io/radius-project/dev/test/testrecipes/test-bicep-recipes/<name>:pr-func6c9ad5ee65
  • 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-func6c9ad5ee65
  • controller test image location: ghcr.io/radius-project/dev/controller:pr-func6c9ad5ee65
  • ucp test image location: ghcr.io/radius-project/dev/ucpd:pr-func6c9ad5ee65
  • deployment-engine test image location: ghcr.io/radius-project/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 ucp-cloud functional tests...
⌛ Starting corerp-cloud functional tests...
⌛ Starting datastoresrp-cloud functional tests...
✅ ucp-cloud functional tests succeeded
✅ corerp-cloud functional tests succeeded
✅ datastoresrp-cloud functional tests succeeded

@radius-functional-tests
Copy link

radius-functional-tests bot commented Sep 27, 2024

Radius functional test overview

🔍 Go to test action run

Name Value
Repository kachawla/radius
Commit ref c06a86b
Unique ID func24847914c8
Image tag pr-func24847914c8
Click here to see the list of tools in the current test run
  • gotestsum 1.12.0
  • KinD: v0.20.0
  • Dapr: 1.12.0
  • Azure KeyVault CSI driver: 1.4.2
  • Azure Workload identity webhook: 1.3.0
  • Bicep recipe location ghcr.io/radius-project/dev/test/testrecipes/test-bicep-recipes/<name>:pr-func24847914c8
  • 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-func24847914c8
  • controller test image location: ghcr.io/radius-project/dev/controller:pr-func24847914c8
  • ucp test image location: ghcr.io/radius-project/dev/ucpd:pr-func24847914c8
  • deployment-engine test image location: ghcr.io/radius-project/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 ucp-cloud functional tests...
⌛ Starting datastoresrp-cloud functional tests...
⌛ Starting corerp-cloud functional tests...
✅ ucp-cloud functional tests succeeded
✅ datastoresrp-cloud functional tests succeeded
✅ corerp-cloud functional tests succeeded

ytimocin
ytimocin previously approved these changes Sep 27, 2024
@radius-functional-tests
Copy link

radius-functional-tests bot commented Oct 1, 2024

Radius functional test overview

🔍 Go to test action run

Name Value
Repository kachawla/radius
Commit ref 31bf25d
Unique ID func52ad013c92
Image tag pr-func52ad013c92
Click here to see the list of tools in the current test run
  • gotestsum 1.12.0
  • KinD: v0.20.0
  • Dapr: 1.12.0
  • Azure KeyVault CSI driver: 1.4.2
  • Azure Workload identity webhook: 1.3.0
  • Bicep recipe location ghcr.io/radius-project/dev/test/testrecipes/test-bicep-recipes/<name>:pr-func52ad013c92
  • 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-func52ad013c92
  • controller test image location: ghcr.io/radius-project/dev/controller:pr-func52ad013c92
  • ucp test image location: ghcr.io/radius-project/dev/ucpd:pr-func52ad013c92
  • deployment-engine test image location: ghcr.io/radius-project/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 corerp-cloud functional tests...
⌛ Starting datastoresrp-cloud functional tests...
⌛ Starting ucp-cloud functional tests...
✅ ucp-cloud functional tests succeeded
✅ datastoresrp-cloud functional tests succeeded
✅ corerp-cloud functional tests succeeded

lakshmimsft
lakshmimsft previously approved these changes Oct 1, 2024
@radius-functional-tests
Copy link

radius-functional-tests bot commented Oct 1, 2024

Radius functional test overview

🔍 Go to test action run

Name Value
Repository kachawla/radius
Commit ref 58fd5cf
Unique ID func5a5109aeab
Image tag pr-func5a5109aeab
Click here to see the list of tools in the current test run
  • gotestsum 1.12.0
  • KinD: v0.20.0
  • Dapr: 1.12.0
  • Azure KeyVault CSI driver: 1.4.2
  • Azure Workload identity webhook: 1.3.0
  • Bicep recipe location ghcr.io/radius-project/dev/test/testrecipes/test-bicep-recipes/<name>:pr-func5a5109aeab
  • 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-func5a5109aeab
  • controller test image location: ghcr.io/radius-project/dev/controller:pr-func5a5109aeab
  • ucp test image location: ghcr.io/radius-project/dev/ucpd:pr-func5a5109aeab
  • deployment-engine test image location: ghcr.io/radius-project/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 ucp-cloud functional tests...
⌛ Starting datastoresrp-cloud functional tests...
✅ ucp-cloud functional tests succeeded
✅ datastoresrp-cloud functional tests succeeded
⌛ Starting corerp-cloud functional tests...
✅ corerp-cloud functional tests succeeded

@kachawla kachawla merged commit ecd79d1 into radius-project:main Oct 3, 2024
28 checks passed
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.

5 participants