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

[KYUUBI #4078] [FOLLOWUP] fix shellcheck violations in scripts of /bin folder #4162

Closed
wants to merge 5 commits into from
Closed

[KYUUBI #4078] [FOLLOWUP] fix shellcheck violations in scripts of /bin folder #4162

wants to merge 5 commits into from

Conversation

davidyuan1223
Copy link
Contributor

@davidyuan1223 davidyuan1223 commented Jan 13, 2023 •
edited by bowenliang123
Loading

Why are the changes needed?

  • fix shellcheck violations in scripts of /bin folder
  • enable shellcheck rule checks

How was this patch tested?

  • Add some test cases that check the changes thoroughly including negative and positive cases if possible

  • Add screenshots for manual tests if appropriate

  • Run test locally before make a pull request

davidyuan1223 and others added 4 commits January 13, 2023 16:40
@davidyuan1223
fix-apache#4057 info:
dd39efd 
1. "$@" is a array, we want use string to compare. so update "$@" => "$*"
2. `tty` mean execute the command, we can use $(tty) replace it
3. param $# is a number, compare number should use -gt/-lt,not >/<
@davidyuan1223
fix-apache#4057 info: modify the shellcheck errors file in ./bin
86e4e1c 
1. "$@" is a array, we want use string to compare. so update "$@" => "$*"
2. `tty` mean execute the command, we can use $(tty) replace it
3. param $# is a number, compare number should use -gt/-lt,not >/<
4. not sure the /bin/kyuubi line 63 'exit -1' need modify? so the directory bin only have a shellcheck note in /bin/kyuubi
@davidyuan1223
Merge remote-tracking branch 'origin/fix-#4057' into fix-apache#4057
cb11935 
# Conflicts:
#	bin/docker-image-tool.sh
@davidyuan1223
Merge pull request #10 from xiaoyuandajian/fix-#4057
55a0a43 
Fix apache#4057
@davidyuan1223 davidyuan1223 reopened this Jan 13, 2023
@bowenliang123
Copy link
Contributor

Thanks for the follow-up PR. And please try to put more details in the PR title and descriptions next time, which are critical for reviewing and feedback. @xiaoyuandajian

bowenliang123
bowenliang123 reviewed Jan 19, 2023
View reviewed changes
bin/docker-image-tool.sh Outdated
@@ -227,8 +227,8 @@ Examples:
EOF
}

# shellcheck disable=SC2199
if [[ "$@" = *--help ]] || [[ "$@" = *-h ]]; then

Copy link
Contributor

Choose a reason for hiding this comment

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

remove the used blank line.

bowenliang123
bowenliang123 reviewed Jan 19, 2023
View reviewed changes
bin/kyuubi Outdated Show resolved Hide resolved
bowenliang123
bowenliang123 reviewed Jan 19, 2023
View reviewed changes
bin/kyuubi-logo Outdated Show resolved Hide resolved
bowenliang123
bowenliang123 reviewed Jan 19, 2023
View reviewed changes
bin/kyuubi-logo Outdated Show resolved Hide resolved
@bowenliang123 bowenliang123 changed the title fix issue #4057 [KYUUBI #4078] [FOLLOWUP] fix shellcheck violations in scripts of /bin folder Jan 19, 2023
@codecov-commenter
Copy link

codecov-commenter commented Mar 5, 2023 •
edited
Loading

Codecov Report

Merging #4162 (c48ad38) into master (03fe8a8) will increase coverage by 0.19%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #4162      +/-   ##
============================================
+ Coverage     53.02%   53.22%   +0.19%     
  Complexity       13       13              
============================================
  Files           548      569      +21     
  Lines         29938    31165    +1227     
  Branches       4023     4210     +187     
============================================
+ Hits          15875    16587     +712     
- Misses        12589    13004     +415     
- Partials       1474     1574     +100
Impacted Files Coverage Δ
...e/kyuubi/jdbc/hive/ClosedOrCancelledException.java 0.00% <0.00%> (-100.00%) ⬇️
.../kyuubi/engine/spark/operation/ExecutePython.scala 0.00% <0.00%> (-81.47%) ⬇️
...e/spark/api/python/KyuubiPythonGatewayServer.scala 0.00% <0.00%> (-73.92%) ⬇️
...ain/scala/org/apache/kyuubi/util/RowSetUtils.scala 26.08% <0.00%> (-57.25%) ⬇️
.../org/apache/kyuubi/server/trino/api/v1/dto/Ok.java 46.15% <0.00%> (-23.08%) ⬇️
...g/apache/kyuubi/client/api/v1/dto/VersionInfo.java 30.76% <0.00%> (-23.08%) ⬇️
...ache/kyuubi/plugin/spark/authz/serde/package.scala 65.62% <0.00%> (-18.75%) ⬇️
.../authentication/EngineSecuritySecretProvider.scala 84.61% <0.00%> (-15.39%) ⬇️
...uthentication/LdapAuthenticationProviderImpl.scala 78.04% <0.00%> (-13.96%) ⬇️
...scala/org/apache/kyuubi/service/ServiceUtils.scala 88.88% <0.00%> (-11.12%) ⬇️
... and 128 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

bowenliang123
bowenliang123 approved these changes Mar 6, 2023
View reviewed changes
Copy link
Contributor

@bowenliang123 bowenliang123 left a comment

Choose a reason for hiding this comment

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

LGTM

bowenliang123
bowenliang123 reviewed Mar 6, 2023
View reviewed changes
bin/kyuubi-logo
@@ -15,18 +15,15 @@
# See the License for the specific language governing permissions and
# limitations under the License.
#

Copy link
Contributor

@bowenliang123 bowenliang123 Mar 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We'd be better to keep this blank line, separating the license block and the single line comment below. @davidyuan1223

@bowenliang123
Copy link
Contributor

Do you consider fixing the disabled SC2242 shellcheck style in bin/kyuubi as well?

@davidyuan1223
Copy link
Contributor Author

Do you consider fixing the disabled SC2242 shellcheck style in bin/kyuubi as well?

I'm not sure which error code should return, so I not fix the bug

@bowenliang123
Copy link
Contributor

Do you consider fixing the disabled SC2242 shellcheck style in bin/kyuubi as well?

I'm not sure which error code should return, so I not fix the bug

Alright, got it.

@davidyuan1223
Copy link
Contributor Author

Do you consider fixing the disabled SC2242 shellcheck style in bin/kyuubi as well?

I'm not sure which error code should return, so I not fix the bug

Alright, got it.

you can choose a 1~255 code, I will use the code to fix the bug

@bowenliang123 bowenliang123 added this to the v1.8.0 milestone Mar 7, 2023
@bowenliang123
Copy link
Contributor

Thanks, merged to master.

@davidyuan1223
Copy link
Contributor Author

Thanks, merged to master.

ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bowenliang123 bowenliang123 bowenliang123 approved these changes

Assignees

@davidyuan1223 davidyuan1223

Labels
kind:build kind:deploy module:kubernetes module:server
Projects
None yet
Milestone
v1.8.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@davidyuan1223 @bowenliang123 @codecov-commenter