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 verify scripts for various checks #21

Merged
merged 1 commit into from
May 10, 2024

Conversation

yuanchen8911
Copy link
Collaborator

@yuanchen8911 yuanchen8911 commented May 9, 2024

Add verification and check scripts: gofmt, golint, spelling check, yaml format, ...
hack/verifiy-all or make verify will perform all checks.
hack/update-all or make update will correct all the issues.

@dmitsh
Copy link
Collaborator

dmitsh commented May 9, 2024

isn't it an overkill?

@yuanchen8911
Copy link
Collaborator Author

yuanchen8911 commented May 10, 2024

isn't it an overkill?

I removed the directory venv which contains a large number of files. We just need the verify and uddatel scripts in the hack folder: a total of 10 checks only.

I think these scripts are quite useful. Running hack/verify-all.sh has identified a few issues in the current code and docs though most of them are minor.

@dmitsh, can you take another look?

@yuanchen8911 yuanchen8911 force-pushed the verification branch 2 times, most recently from 70d7ef7 to 2a6aa78 Compare May 10, 2024 01:05
@yuanchen8911
Copy link
Collaborator Author

/retest

@yuanchen8911 yuanchen8911 force-pushed the verification branch 2 times, most recently from eb9ecf3 to 6aca469 Compare May 10, 2024 04:17
@yuanchen8911 yuanchen8911 force-pushed the verification branch 2 times, most recently from 3829ea8 to a56ecc4 Compare May 10, 2024 16:36
@yuanchen8911 yuanchen8911 requested review from dmitsh and shinae-woo May 10, 2024 16:36
@yuanchen8911 yuanchen8911 force-pushed the verification branch 2 times, most recently from 48a3a9c to 7ca2fcc Compare May 10, 2024 16:45
@yuanchen8911 yuanchen8911 changed the title Add verification scripts Add verify scripts for various checks. May 10, 2024
@yuanchen8911 yuanchen8911 changed the title Add verify scripts for various checks. Add verify scripts for various checks May 10, 2024
@shinae-woo
Copy link
Collaborator

I have one issue on yaml format, which I can ignore.

shinaew@shinae-sjc-desktop:~/src/knavigator$ make update
[*] Update go format...
Update go format
[*] Update go mod...
[*] Update go lint...
[*] Update ends newline...
[*] Update shell format...
Update shell format
[*] Update yaml format...
Update yaml format
2024/05/10 12:34:43 encountered the following formatting errors:
./charts/virtual-nodes/templates/nodes.yaml: yaml: line 3: did not find expected alphabetic or numeric character
exit status 1
[*] Update spelling...
Update failed for: yaml-format
make: *** [Makefile:33: update] Error 1

And two new lines at end of file

diff --git a/.github/workflows/docker.yml b/.github/workflows/docker.yml
index 86f894f..134ab8d 100644
--- a/.github/workflows/docker.yml
+++ b/.github/workflows/docker.yml
@@ -60,4 +60,4 @@ jobs:
           context: .
           push: ${{ github.event_name != 'pull_request' }}
           tags: ${{ steps.meta.outputs.tags }}
-          labels: ${{ steps.meta.outputs.labels }}
\ No newline at end of file
+          labels: ${{ steps.meta.outputs.labels }}
diff --git a/docs/task_management.md b/docs/task_management.md
index fb84593..d24157f 100644
--- a/docs/task_management.md
+++ b/docs/task_management.md
@@ -8,4 +8,4 @@ Tasks are defined in a YAML format or received via HTTP/gRPC protocols. The Task
 - Check the spec and status of nodes
 - Check the spec and status of pods
 - Run PromQL query
-- Sleep for a specified duration
\ No newline at end of file
+- Sleep for a specified duration

Copy link
Collaborator

@shinae-woo shinae-woo left a comment

Choose a reason for hiding this comment

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

LGTM other than issues I reported on above comments

@yuanchen8911
Copy link
Collaborator Author

LGTM other than issues I reported on above comments

Thanks! Not sure how to fix the first one. #28 should fix the empty line issues.

Signed-off-by: Yuan Chen <[email protected]>

Add verify and update to Makefile

Signed-off-by: Yuan Chen <[email protected]>

Remove venv

Revert changes

Signed-off-by: Yuan Chen <[email protected]>

Add yamlfmt

Signed-off-by: Yuan Chen <[email protected]>

Remove golangci.yaml

Fix goling config file names
@yuanchen8911 yuanchen8911 merged commit 144e7a7 into NVIDIA:main May 10, 2024
4 checks passed
yuanchen8911 added a commit to yuanchen8911/knavigator that referenced this pull request May 10, 2024
This reverts commit 144e7a7, reversing
changes made to 2f29e50.
yuanchen8911 added a commit to yuanchen8911/knavigator that referenced this pull request May 10, 2024
This reverts commit 144e7a7, reversing
changes made to 2f29e50.

Signed-off-by: Yuan Chen <[email protected]>
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.

3 participants