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.

Sign up for GitHub

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

Jump to bottom

CNDB-9877: Update PR template #1399

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

CNDB-9877: Update PR template #1399

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions .github/pull_request_template.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,12 @@
...

### Checklist before you submit for review
- [ ] Make sure there is a PR in the CNDB project updating the Converged Cassandra version
- [ ] Make sure there is a PR in the CNDB project updating the Converged Cassandra version if your PR is changing the API
Copy link

@sbtourist sbtourist Oct 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
- [ ] Make sure there is a PR in the CNDB project updating the Converged Cassandra version if your PR is changing the API
- [ ] Make sure there is a PR in the CNDB project updating the Converged Cassandra version if your PR affects CNDB.

The question is, how do we know the PR affects CNDB? For example it could be a HCD change and indirectly affect CNDB. What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

isn't a similar problem as we have with Stargate-CNDB relation? Stargate uses CNDB coordinator as an impl so we run stargate internal tests against that implementation;

I think the bare minimum would be to compile CNDB against the CC artifact produced in from the feature branch as a step in CC gating

- [ ] Use `NoSpamLogger` for log lines that may appear frequently in the logs
- [ ] Verify test results on Butler
- [ ] Test coverage for new/modified code is > 80%
- [ ] Verify Butler and Sonar checks are passing
- [ ] Proper code formatting
- [ ] Proper title for each commit staring with the project-issue number, like CNDB-1234
- [ ] Each commit has a meaningful description
- [ ] Each commit is not very long and contains related changes
- [ ] Renames, moves and reformatting are in distinct commits
- [ ] Each commit contains related changes
Copy link

@sbtourist sbtourist Oct 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
- [ ] Each commit contains related changes
- [ ] Each commit is specific to the issue it's addressing

- [ ] Renames, moves and reformatting are in distinct commits
Copy link

Choose a reason for hiding this comment

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

I would still drop this one, as I think most of the time it would generate more - artificially separated - work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can drop it, but it is just a friendly reminder - personally I'd not start reviewing a PR which mixes renamings and major reformattings with the code changes - just with "how to write tests" - it is just sharing "how to"

- [ ] Avoid starting nodes in new tests (jvm dtests, CQLTester) if possible in favor of implementing pure unit tests
Copy link

Choose a reason for hiding this comment

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

I don't quite agree with this: in a complex distributed system, integration tests are fundamental and should not be discouraged even if unit tests exist.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

though starting nodes over and over again should be avoided - if we are adding a test case to the test class which already has some shared containers running - that's fine; however we should design tests having the testing time in mind;

Copy link

Choose a reason for hiding this comment

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

Agreed, but this is more of a general "know how to write efficient tests" issue?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

indeed; though PR template seems to be an efficient way to share that knowledge to the most appropriate audience, isn't it?