Coding guidelines for external contributions #2634
Conversation
ashmeenkaur
force-pushed
the
coding_guidelines
branch
2 times, most recently
from
a145ccc
to
4b260a1
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2634 +/- ##
==========================================
- Coverage 77.58% 77.55% -0.04%
==========================================
Files 109 109
Lines 15555 15555
==========================================
- Hits 12068 12063 -5
- Misses 2973 2977 +4
- Partials 514 515 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
kislaykishore
requested review from
a team,
kislaykishore and
gargnitingoogle
and removed request for
a team and
kislaykishore
docs/dev_guide.md
Outdated
| help output. | ||
| * This signals to users that the feature is in development and may change or be | ||
| removed. | ||
| * After the feature development is complete, developers should open a GitHub |
There was a problem hiding this comment.
Also should add some rationale on when to add composite or end to end tests.
There was a problem hiding this comment.
I think there is no clear distinction and both needs to be added for new features. I've added some details, PTAL.
docs/dev_guide.md
Outdated
| GODEBUG=asyncpreemptoff=1 CGO_ENABLED=0 go test ./tools/integration_tests/$TEST_PACKAGE_NAME/... -p 1 -short --integrationTest -v --testbucket=$TEST_BUCKET_NAME --timeout=60m -run $TEST_NAME | ||
| ``` | ||
| 4. **Run all tests as pre-submit:** You can run existing GCSFuse end-to-end | ||
| tests as a pre-submit by adding `execute-integration-tests` label to the pull |
There was a problem hiding this comment.
External user will not be able to add any label on the PR. We have to provide go test command or steps to run our e2e test script locally.
Alternatively, If this is possible we could configure a system to automatically apply the label when a pull request (PR) is marked as "ready for review." e.g. https://github.com/apps/trusted-contributions-gcf
There was a problem hiding this comment.
Thanks for catching this. I will change to "Ask GCSFuse team to apply execute-integration-tests label."
There was a problem hiding this comment.
Although they'll not be able to see which test fails on their PR. They won't be able to access fusion link. Better to share command to tests on them self
|
|
||
| Rollout of any new feature should be done in 2 phases: | ||
|
|
||
| **1. Experimental Phase:** |
There was a problem hiding this comment.
Where will we keep/document features in experimental phase and their flags? I dont want to document them on public docs, so can we have a doc under contributing that shows all contributed features and their status? And ask users to update that as well?
There was a problem hiding this comment.
IMO, this yaml file (which we use to generate flag/config code) should serve the purpose of documentation too: https://github.com/GoogleCloudPlatform/gcsfuse/blob/master/cfg/params.yaml
"experimental" prefix in the flag-name: and hide-flag: parameter should convey the status.
Tulsishah
added
execute-package-build-tests
ashmeenkaur
force-pushed
the
coding_guidelines
branch
from
57d4a40
to
6ce6616
Compare
ashmeenkaur
force-pushed
the
coding_guidelines
branch
from
6ce6616
to
afac008
Compare
|
|
||
| **1. Experimental Phase:** | ||
|
|
||
| * New features should initially be introduced behind an experimental flag. These |
There was a problem hiding this comment.
Can we add make some text here to link to the section below of Adding a CLI/Config flag.
| GCSFuse even during the experimental phase. This helps ensure the | ||
| feature's stability, identify potential issues early, and prevent | ||
| regressions. | ||
| * Add unit tests for all new code written for the experimental feature. This |
There was a problem hiding this comment.
Add link to the section below of unit tests
| regressions. | ||
| * Add unit tests for all new code written for the experimental feature. This | ||
| includes testing edge cases, boundary conditions, and error handling. | ||
| * Add composite tests to validate GCSFuse's end user functionality while |
There was a problem hiding this comment.
Similar to composite and end to end.
| mainline, the feature will roll out as an experimental feature in the next | ||
| GCSFuse release with the feature disabled by default. | ||
|
|
||
| * Developers should open a GitHub issue ( |
There was a problem hiding this comment.
Should the developers create a new github issue in addition to the one they started created before they started the experimental changes?
Description
Added coding guidelines for external contributions.
Link to the issue in case of a bug fix.
NA
Testing details