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

docs(contributing): revamp contributing docs #7290

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

grandwizard28
Copy link
Collaborator

@grandwizard28 grandwizard28 commented Mar 12, 2025

Summary

Related Issues / PR's

Screenshots

NA

Affected Areas and Manually Tested Areas


Important

Revamped contributing docs, added Docker setup for ClickHouse, and enhanced Makefile for streamlined development.

  • Documentation:
    • Revamped CONTRIBUTING.md to provide clearer guidelines on how to contribute, report bugs, and open pull requests.
    • Added docs/contributing/development.md to guide setting up a local development environment, including prerequisites and build instructions.
  • Docker Setup:
    • Added .devenv/docker/clickhouse/compose.yaml for setting up ClickHouse with Docker, including services for ClickHouse, Zookeeper, and schema migrators.
    • Configured ClickHouse server settings in config.xml and user settings in users.xml.
  • Makefile Enhancements:
    • Added devenv-clickhouse target to start ClickHouse in development environment.
    • Added run-go target to run the Go backend server with specific environment variables.

This description was created by Ellipsis for 94c663c. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Reviewed everything up to 26c1384 in 3 minutes and 8 seconds

More details
  • Looked at 725 lines of code in 6 files
  • Skipped 1 files when reviewing.
  • Skipped posting 16 drafted comments based on config settings.
1. .devenv/docker/clickhouse/compose.yaml:52
  • Draft comment:
    Verify that the '--up=' argument in the schema migrator commands is correctly set; an empty parameter may be unintended.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is asking the PR author to verify their intention, which violates the rule against asking for confirmation or verification. It does not provide a specific suggestion or point out a clear issue with the code.
2. .devenv/docker/clickhouse/compose.yaml:16
  • Draft comment:
    Ensure the healthcheck command using 'wget' is supported in the chosen alpine image; consider using 'curl' if necessary.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  1. The comment violates the rule about not asking for verification ("Ensure that..."). 2. Alpine Linux typically includes wget by default. 3. The ClickHouse official alpine image likely has the necessary tools for its own healthcheck. 4. The comment is speculative without evidence that wget is actually missing. 5. If wget were missing, this would fail immediately in testing/deployment.
    The image could potentially be customized to exclude wget, and curl might be more commonly available.
    The official ClickHouse alpine image is a controlled environment, and if wget were missing, it would be immediately apparent during testing - no need for a speculative comment.
    Delete the comment as it violates the rule about verification requests and makes a speculative suggestion without evidence of an actual problem.
3. .devenv/docker/clickhouse/compose.yaml:52
  • Draft comment:
    The schema-migrator commands include an empty '--up=' flag. It would be helpful to add a comment explaining the purpose of this flag to avoid confusion for future maintainers.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    While documentation can be helpful, this seems like a purely informative comment that doesn't require any actual code changes. The empty flag is likely a valid configuration option, and if it was incorrect, it would be caught during testing. The comment is more about explaining existing behavior rather than fixing an issue.
    The flag's purpose might not be obvious to new developers, and documentation could save time for future maintainers. Missing documentation could lead to confusion.
    Per the rules, we should not make purely informative comments. If the empty flag was problematic, it would be caught in testing. Documentation belongs in proper documentation files, not inline comments.
    Delete the comment as it is purely informative and doesn't require any actual code changes.
4. .devenv/docker/clickhouse/compose.yaml:38
  • Draft comment:
    The zookeeper healthcheck uses a curl command against 'http://localhost:8080/commands/ruok'. Please verify that port 8080 is correct and that the zookeeper container exposes the expected admin interface on that port.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is asking the PR author to verify the port and interface, which violates the rule against asking for confirmation or verification. It does not provide a specific suggestion or point out a clear issue.
5. .devenv/docker/clickhouse/fs/etc/clickhouse-server/users.d/users.xml:19
  • Draft comment:
    The 'access_management' field is duplicated in the default user configuration. Consider removing the duplicate entry to avoid potential configuration conflicts.
  • Reason this comment was not posted:
    Marked as duplicate.
6. Makefile:117
  • Draft comment:
    The 'dev-setup' target has been removed. Please ensure that all documentation and developer workflow references to this target are updated accordingly, to prevent confusion for contributors accustomed to the old target.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. docs/contributing/development.md:32
  • Draft comment:
    The development guide looks clear. Just ensure that it remains in sync with updates in CONTRIBUTING.md and the Makefile so that new contributors have accurate instructions.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is purely informative and asks the PR author to ensure that documentation remains in sync, which violates the rule against asking the author to ensure things are tested or confirmed. It does not provide a specific code suggestion or highlight a specific issue with the code.
8. .devenv/docker/clickhouse/fs/etc/clickhouse-server/config.d/config.xml:47
  • Draft comment:
    The file does not end with a newline. Please add a newline at the end of the file to follow standard conventions.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is purely informative and does not provide a specific code suggestion or address a potential issue. It violates the rule against making purely informative comments.
9. CONTRIBUTING.md:60
  • Draft comment:
    Typo: Consider changing 'additional document' to 'additional documentation' for clarity.
  • Reason this comment was not posted:
    Comment was on unchanged code.
10. CONTRIBUTING.md:71
  • Draft comment:
    Typographical suggestion: Consider revising 'multiple sub-component' to 'multiple subcomponents' (or 'multiple sub-components') to improve readability.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. CONTRIBUTING.md:88
  • Draft comment:
    Typographical error: It appears 'See up your' is a mistake. Perhaps it should be 'Set up your' to make the instruction clear.
  • Reason this comment was not posted:
    Marked as duplicate.
12. docs/contributing/development.md:17
  • Draft comment:
    In the sentence on line 17, consider changing 'setup local development environments' to 'set up local development environments' for grammatical correctness.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    While the grammar correction is technically correct, the rules state not to make purely informative comments or comments that are obvious/unimportant. This is a minor grammatical issue in documentation that doesn't affect functionality or understanding. The meaning is still clear despite the grammatical error.
    The grammar mistake could potentially confuse non-native English speakers. Documentation quality is important for project professionalism.
    While documentation quality matters, this is an extremely minor issue that doesn't impact comprehension. The rules specifically say to avoid purely informative comments.
    The comment should be deleted as it violates the rule about not making purely informative comments that don't require code changes.
13. docs/contributing/development.md:19
  • Draft comment:
    On line 19, the phrase 'print most of available commands' would read better as 'print most of the available commands'. Please add 'the' for clarity.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    While the grammar suggestion is technically correct, it's an extremely minor change that doesn't impact understanding. The rules don't specifically address documentation grammar fixes, but they emphasize only keeping comments that require clear code changes. Documentation readability is important, but this change is too trivial.
    Documentation quality and readability do matter for open source projects. Poor grammar could reflect badly on the project.
    While documentation quality matters, this change is so minor that it creates more noise than value in the review process. The current phrasing is perfectly understandable.
    Delete this comment as it suggests an extremely minor grammar change that doesn't meaningfully impact documentation readability or understanding.
14. docs/contributing/development.md:44
  • Draft comment:
    On line 44, consider capitalizing 'apis' to 'APIs' to match common usage and improve clarity.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
    While technically correct that 'APIs' is the more common capitalization, this is a minor documentation style issue. The document is perfectly understandable as is. The rules state not to make purely informative comments or comments that are obvious/unimportant. This seems to fall into that category.
    The capitalization of technical acronyms can affect readability and professionalism of documentation. Multiple style guides recommend capitalizing API.
    While proper capitalization is good practice, this comment doesn't highlight a critical issue that would impede understanding or functionality. It's more of a nitpick.
    Delete the comment as it's too minor and falls under the category of "obvious or unimportant" changes that the rules say to avoid.
15. docs/contributing/development.md:52
  • Draft comment:
    On line 52, 'api' should be capitalized to 'API' for consistency and clarity.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    This is a documentation file, so technical accuracy and consistency are important. However, the file itself is not consistent with API capitalization. The comment is about a very minor style issue that doesn't affect understanding. The rules say not to make purely informative comments or comments that are obvious/unimportant.
    The capitalization of technical terms could be important for maintaining professional documentation standards. Inconsistent capitalization throughout a document could be confusing for readers.
    While consistency is good, this single instance of capitalization is too minor to warrant a PR comment. If capitalization standardization is needed, it should be handled through a style guide or separate documentation review.
    Delete this comment as it's too minor and purely stylistic, not affecting the actual functionality or clarity of the documentation.
16. docs/contributing/development.md:71
  • Draft comment:
    On line 71, consider changing 'backend api endpoint' to 'backend API endpoint' for consistency.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    While technical writing best practices often capitalize API, this is a minor stylistic issue in documentation. The comment doesn't point out a code issue, security concern, or functional problem. Documentation style consistency, while nice to have, isn't critical enough to warrant a PR comment based on our rules about not making purely informative comments.
    The capitalization of technical terms could affect readability and professionalism of documentation. Multiple instances of lowercase 'api' in the file suggest a broader consistency issue.
    While documentation consistency is valuable, this level of nitpicking violates our rule about not making purely informative comments that don't require clear code changes.
    Delete the comment as it's a minor documentation style suggestion that doesn't meet our threshold for required PR comments.

Workflow ID: wflow_TLAe8axehawzZGf4


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.


Thank You!
- See up your [development](docs/contributing/development.md) environment.
Copy link
Contributor

Choose a reason for hiding this comment

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

The phrase 'See up your development environment.' is a bit awkward. Consider rewording it to something like 'Set up your development environment' for better clarity.

Suggested change
- See up your [development](docs/contributing/development.md) environment.
- Set up your [development](docs/contributing/development.md) environment.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on 02a0c3e in 1 minute and 13 seconds

More details
  • Looked at 24 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 6 drafted comments based on config settings.
1. CONTRIBUTING.md:1
  • Draft comment:
    The updated headings improve clarity by removing the welcome emoji and restructuring the headers. Please double-check that the markdown formatting (spacing and line breaks) remains consistent throughout the document.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
2. CONTRIBUTING.md:56
  • Draft comment:
    Consider replacing 'additional document' with 'additional documentation' for clarity and consistency.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. CONTRIBUTING.md:84
  • Draft comment:
    Typo detected: 'See up your [development]' likely should be 'Set up your [development] environment.'
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. CONTRIBUTING.md:56
  • Draft comment:
    On line 56, the sentence 'GitHub provides additional document on...' seems odd. Consider changing 'additional document' to 'additional documentation' to improve clarity.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. CONTRIBUTING.md:67
  • Draft comment:
    On line 67, the phrase 'multiple sub-component' appears. It might be clearer to use 'multiple sub-components' (with an 's') to indicate plurality.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. CONTRIBUTING.md:84
  • Draft comment:
    On line 84, the text '- See up your development environment.' seems like it might be a typo. Did you mean 'Set up your development environment'?
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_QAErYyksymN6s3ZD


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on 7167e4a in 38 seconds

More details
  • Looked at 12 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. .devenv/docker/clickhouse/fs/etc/clickhouse-server/users.d/users.xml:18
  • Draft comment:
    Removing <access_management>1</access_management> may affect default user privileges. Ensure this is intentional and the impact on user roles is evaluated.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. .devenv/docker/clickhouse/fs/etc/clickhouse-server/users.d/users.xml:18
  • Draft comment:
    Ensure removal of <access_management> is intentional. This change alters default user privileges and might affect access management functionality. Update docs if needed.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_tSI6PddgRFqOp1FP


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on 94c663c in 32 seconds

More details
  • Looked at 84 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. pkg/query-service/README.md:1
  • Draft comment:
    Removal of the entire README. Ensure that necessary documentation isn’t lost; if still required, relocate or update references accordingly.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. pkg/query-service/README.md:1
  • Draft comment:
    The removal of the Query Service README deletes setup and configuration instructions. Ensure that any critical information (e.g., local ClickHouse setup, backend config, and build steps) is relocated or referenced in the new contributing docs so developers aren’t left without guidance.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None

Workflow ID: wflow_1acaIL1hDgIWLakx


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

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.

1 participant