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]: Implement CI validation of shell scripts using shellcheck #4822

Closed
yurishkuro opened this issue Oct 7, 2023 · 3 comments · Fixed by #4826
Closed

[Feature]: Implement CI validation of shell scripts using shellcheck #4822

yurishkuro opened this issue Oct 7, 2023 · 3 comments · Fixed by #4826
Labels
enhancement good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement

Comments

@yurishkuro
Copy link
Member

According to its docs, the shellcheck tool already comes pre-installed in Github runners, so we could add a workflow that will validate all build scripts we use.

Currently running it against our scripts produces 83 warnings, so first we'd need to review them and fix the issues (or disable some lint checks), before we can enable the linter in CI.

$ shellcheck scripts/*sh | grep '^In ' | wc -l
      83
@yurishkuro yurishkuro added enhancement help wanted Features that maintainers are willing to accept but do not have cycles to implement good first issue Good for beginners labels Oct 7, 2023
This was referenced Oct 7, 2023
Merged
Merged
yurishkuro added a commit that referenced this issue Oct 8, 2023
@yurishkuro
Clean-up some linter warnings in build scripts (#4823)
6e90a49 
## Which problem is this PR solving?
- Part of #4822

## Description of the changes
- Address some linter warnings

## How was this change tested?
- CI

---------

Signed-off-by: Yuri Shkuro <github@ysh.us>
@akagami-harsh
Copy link
Member

hi @yurishkuro i would like to take on this issue, i am a beginner here so can you please guide how do i approach this issue

@akagami-harsh
Copy link
Member

@yurishkuro which issues should i fix and on which should i disable lint checks

akagami-harsh added a commit to akagami-harsh/jaeger that referenced this issue Oct 8, 2023
@akagami-harsh
Fixes jaegertracing#4822: fixed all warnning produced by shellcheck
13a071d 
Signed-off-by: Harshvir Potpose <hpotpose62@gmail.com>
@akagami-harsh akagami-harsh mentioned this issue Oct 8, 2023
Merged
4 tasks
@satyazzz123
Copy link

@yurishkuro can I also work on this issue ?

yurishkuro added a commit that referenced this issue Oct 19, 2023
@akagami-harsh @yurishkuro
Add CI validation of shell scripts using shellcheck (#4826)
1b6fecc 
## Which problem is this PR solving?
Resolves #4822 

## Description of the changes
- fixed all 83 warnings produced by shellcheck
- added a workflow for shellcheck

## How was this change tested?
- CI

## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [ ] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `yarn lint` and `yarn test`

---------

Signed-off-by: Harshvir Potpose <hpotpose62@gmail.com>
Signed-off-by: Harshvir Potpose <122517264+akagami-harsh@users.noreply.github.com>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
Co-authored-by: Yuri Shkuro <github@ysh.us>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add CI validation of shell scripts using shellcheck akagami-harsh/jaeger
3 participants
@yurishkuro @satyazzz123 @akagami-harsh