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

[FEATURE] Code analysis tool #313

Merged
merged 3 commits into from
May 30, 2023
Merged

[FEATURE] Code analysis tool #313

merged 3 commits into from
May 30, 2023

Conversation

zethuman
Copy link
Contributor

Description

This PR adds option to check overall quality of code.

Issues Resolved

Closes [#307].

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

dblock
dblock reviewed Apr 27, 2023
View reviewed changes
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

So how does this work and what does it do? Any badges for README?

.github/workflows/code-analysis.yml Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
github-advanced-security[bot]
github-advanced-security bot found potential problems Apr 28, 2023
View reviewed changes
.github/workflows/code-analysis.yml Outdated
@@ -0,0 +1,34 @@
name: Codacy Analysis CLI

Check notice

Code scanning / Checkov (reported by Codacy)

Ensure top-level permissions are not set to write-all

Ensure top-level permissions are not set to write-all
github-advanced-security[bot]
github-advanced-security bot found potential problems Apr 28, 2023
View reviewed changes
README.md Fixed Show fixed Hide fixed
README.md Fixed Show resolved Hide resolved
@dblock
Copy link
Member

dblock commented Apr 28, 2023

I'm good with this! Do you want to try to address the above complaints from codacy before merging or leave them alone/dismiss them?

dblock
dblock previously approved these changes Apr 28, 2023
View reviewed changes
@dblock
Copy link
Member

dblock commented Apr 28, 2023

@Jakob3xD are you ok with adding codacy here? any reasons not to?

@Jakob3xD
Copy link
Collaborator

@Jakob3xD are you ok with adding codacy here? any reasons not to?

I added my concerns to the issue but in general I am okay with merging it and seeing how its works.

@zethuman
Copy link
Contributor Author

zethuman commented May 1, 2023

Tasks fail due to problems on the side of the tool itself. Here is the link to the issue, I will follow the fix until I need to merge

@zethuman
Copy link
Contributor Author

zethuman commented May 2, 2023

It seems that the problem has been solved, who has access to restart jobs, please restart

github-advanced-security[bot]
github-advanced-security bot found potential problems May 2, 2023
View reviewed changes
README.md Outdated
@@ -1,4 +1,11 @@
[![Go Reference](https://pkg.go.dev/badge/github.com/opensearch-project/opensearch-go.svg)](https://pkg.go.dev/github.com/opensearch-project/opensearch-go/v2) [![Build](https://github.com/opensearch-project/opensearch-go/actions/workflows/lint.yml/badge.svg)](https://github.com/opensearch-project/opensearch-go/actions/workflows/lint.yml) [![Unit](https://github.com/opensearch-project/opensearch-go/actions/workflows/test-unit.yml/badge.svg)](https://github.com/opensearch-project/opensearch-go/actions/workflows/test-unit.yml) [![Integration](https://github.com/opensearch-project/opensearch-go/actions/workflows/test-integration.yml/badge.svg)](https://github.com/opensearch-project/opensearch-go/actions/workflows/test-integration.yml) [![codecov](https://codecov.io/gh/opensearch-project/opensearch-go/branch/main/graph/badge.svg?token=MI9g3KYHVx)](https://codecov.io/gh/opensearch-project/opensearch-go) [![Chat](https://img.shields.io/badge/chat-on%20forums-blue)](https://discuss.opendistrocommunity.dev/c/clients/) ![PRs welcome!](https://img.shields.io/badge/PRs-welcome!-success)
[![Go Reference](https://pkg.go.dev/badge/github.com/opensearch-project/opensearch-go.svg)](https://pkg.go.dev/github.com/opensearch-project/opensearch-go/v2)

Check warning

Code scanning / Markdownlint (reported by Codacy)

First line in a file should be a top-level heading

First line in a file should be a top-level heading
dblock
dblock reviewed May 3, 2023
View reviewed changes
README.md Outdated
**opensearch-go** is
[a community-driven, open source fork](https://aws.amazon.com/blogs/opensource/introducing-opensearch/)
of go-elasticsearch licensed under the [Apache v2.0 License](LICENSE.txt). For
more information, see [opensearch.org](https://opensearch.org/).
Copy link
Member

Choose a reason for hiding this comment

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

This seems unrelated, and can we please not fold markdown?

https://code.dblock.org/2021/06/07/to-wrap-or-not-to-wrap-in-markdown.html

github-advanced-security[bot]
github-advanced-security bot found potential problems May 4, 2023
View reviewed changes
README.md Outdated
@@ -31,7 +39,9 @@

## Code of Conduct

This project has adopted the [Amazon Open Source Code of Conduct](CODE_OF_CONDUCT.md). For more information see the [Code of Conduct FAQ](https://aws.github.io/code-of-conduct-faq), or contact [opensource-codeofconduct@amazon.com](mailto:opensource-codeofconduct@amazon.com) with any additional questions or comments.
This project has adopted the [Amazon Open Source Code of Conduct](CODE_OF_CONDUCT.md). For more information see the [Code of Conduct FAQ]

Check notice

Code scanning / Remark-lint (reported by Codacy)

Warn when references to undefined definitions are found.

[no-undefined-references] Found reference to undefined definition
github-advanced-security[bot]
github-advanced-security bot found potential problems May 4, 2023
View reviewed changes
README.md Outdated
@@ -31,7 +39,9 @@

## Code of Conduct

This project has adopted the [Amazon Open Source Code of Conduct](CODE_OF_CONDUCT.md). For more information see the [Code of Conduct FAQ](https://aws.github.io/code-of-conduct-faq), or contact [opensource-codeofconduct@amazon.com](mailto:opensource-codeofconduct@amazon.com) with any additional questions or comments.
This project has adopted the [Amazon Open Source Code of Conduct](CODE_OF_CONDUCT.md). For more information see the [Code of Conduct FAQ]

Check warning

Code scanning / Markdownlint (reported by Codacy)

Expected: 80; Actual: 137

Expected: 80; Actual: 137
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to disable all or these?

README.md Outdated
@@ -31,7 +39,9 @@

## Code of Conduct

This project has adopted the [Amazon Open Source Code of Conduct](CODE_OF_CONDUCT.md). For more information see the [Code of Conduct FAQ](https://aws.github.io/code-of-conduct-faq), or contact [opensource-codeofconduct@amazon.com](mailto:opensource-codeofconduct@amazon.com) with any additional questions or comments.
This project has adopted the [Amazon Open Source Code of Conduct](CODE_OF_CONDUCT.md). For more information see the [Code of Conduct FAQ]
(https://aws.github.io/code-of-conduct-faq), or contact [opensource-codeofconduct@amazon.com](mailto:opensource-codeofconduct@amazon.com)

Check warning

Code scanning / Markdownlint (reported by Codacy)

Bare URL used

Bare URL used
README.md Outdated
@@ -12,7 +19,8 @@

## Welcome!

**opensearch-go** is [a community-driven, open source fork](https://aws.amazon.com/blogs/opensource/introducing-opensearch/) of go-elasticsearch licensed under the [Apache v2.0 License](LICENSE.txt). For more information, see [opensearch.org](https://opensearch.org/).
**opensearch-go** is
[a community-driven, open source fork](https://aws.amazon.com/blogs/opensource/introducing-opensearch/) of go-elasticsearch licensed under the [Apache v2.0 License](LICENSE.txt). For more information, see [opensearch.org](https://opensearch.org/).

Check warning

Code scanning / Markdownlint (reported by Codacy)

Expected: 80; Actual: 247

Expected: 80; Actual: 247
dblock
dblock reviewed May 4, 2023
View reviewed changes
README.md Outdated
@@ -31,7 +39,9 @@ OpenSearch Go Client

## Code of Conduct

This project has adopted the [Amazon Open Source Code of Conduct](CODE_OF_CONDUCT.md). For more information see the [Code of Conduct FAQ](https://aws.github.io/code-of-conduct-faq), or contact [opensource-codeofconduct@amazon.com](mailto:opensource-codeofconduct@amazon.com) with any additional questions or comments.
This project has adopted the [Amazon Open Source Code of Conduct](CODE_OF_CONDUCT.md). For more information see the [Code of Conduct FAQ]
Copy link
Member

Choose a reason for hiding this comment

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

This still looks wrapped.

@zethuman
Copy link
Contributor Author

zethuman commented May 13, 2023 •
edited
Loading

@dblock I research this topic more deeply, and as far as I understood, large tools are not particularly suitable for us. SonarQube, for example, is practically the most popular solution, but integration with SonarCloud is unlikely to please the community. And the solution with codacy actually turned out to be not so subject to customization. e.g. I could not turn off the check for markdown wrapping. So maybe we should stick with golint for now, as suggested by @Jakob3xD?
Here you can see my experiments

The most interesting thing to note integration with github

image

@dblock
Copy link
Member

dblock commented May 15, 2023

I'm good with whatever you and @Jakob3xD can agree on!

@Jakob3xD
Copy link
Collaborator

@zethuman can you please rebase and maybe remove the changes from README.md.

@zethuman zethuman closed this May 17, 2023
@zethuman zethuman force-pushed the feature/workflow/code_analyzer branch from 9f59154 to 5831f1f Compare May 17, 2023 01:33
@zethuman
add code analysis tool
0efeb4e 
Signed-off-by: Rakhat Zhuman <zhumanrakhat01@gmail.com>
@zethuman
Copy link
Contributor Author

@Jakob3xD done

@zethuman zethuman reopened this May 17, 2023
@zethuman
increased timeout
873135f 
Signed-off-by: Rakhat Zhuman <zhumanrakhat01@gmail.com>
@zethuman
Copy link
Contributor Author

@Jakob3xD @dblock what about this PR? Is this acceptable or does it need to be improved?

@Jakob3xD
Copy link
Collaborator

The MR it self LGTM but I am concerned about the current linting issues. Will the current complains occur on every PR that gets created?

@zethuman
Copy link
Contributor Author

Will the current complains occur on every PR that gets created?

@Jakob3xD no, it is possible to remove it by specifying an optional parameter

# Optional: show only new issues if it's a pull request. The default value is `false`.
# only-new-issues: true

It also seems to me that it is a little illogical to raise the entire project and show all the issues

@zethuman
add only-new-issues paramter
2a95d5e 
Signed-off-by: Rakhat Zhuman <zhumanrakhat01@gmail.com>
@zethuman
Copy link
Contributor Author

@Jakob3xD @dblock can you merge this MR?

@dblock dblock merged commit 602fa25 into opensearch-project:main May 30, 2023
@tannerjones4075 tannerjones4075 mentioned this pull request Oct 31, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dblock dblock dblock approved these changes

@VijayanB VijayanB Awaiting requested review from VijayanB VijayanB is a code owner

@VachaShah VachaShah Awaiting requested review from VachaShah VachaShah is a code owner

@Jakob3xD Jakob3xD Awaiting requested review from Jakob3xD Jakob3xD is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zethuman @dblock @Jakob3xD