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

[CBRD-25760] github memory monitoring check bug fix #5757

Merged
merged 2 commits into from
Jan 6, 2025

Conversation

H2SU
Copy link
Contributor

@H2SU H2SU commented Jan 2, 2025

http://jira.cubrid.org/browse/CBRD-25760

Purpose

  • 문제 상황
    • grep에 -F 옵션을 추가하는 기존 PR에서도 예외상황이 발생
    • compile.c 파일을 grep -F로 검색했을 시 존재하지 않아야 정상인데 compile.cpp가 존재하기 때문에 존재하는 것으로 인식

Implementation

  • grep 옵션 수정

@H2SU H2SU self-assigned this Jan 2, 2025

# Check if the file is in the exception list
for exception in "${EXCEPTION_FILES[@]}"; do
if [ "$f" = "$exception" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

동등 비교 연산자는 ==

Copy link
Contributor

@vimkim vimkim Jan 5, 2025

Choose a reason for hiding this comment

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

안녕하세요, CTP를 디버깅할 때 해당 이슈로 고생했던 기억이 있어서 댓글을 달아보겠습니다. 😊

쉘에 대해 깊이 알지는 못하지만, 쉘 스크립팅에서 비교 연산자 '==' 는 Bash의 고유 문법이며, POSIX 표준에서는 지원되지 않는 것으로 알고 있습니다. POSIX 호환성을 고려한다면 '=' 연산자를 사용하는 것이 더 안전할 수 있다고 생각합니다. 특히 우분투 등 많은 리눅스 환경에서는 초기 부팅 속도 향상을 위해 /bin/sh가 Bash가 아닌 (엄격한 posix 문법으로 구현된) Dash와 같은 lightweight shell에 링크되어 있어, 로컬에서 테스팅할 경우 작동하지 않을 수도 있습니다.

https://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions#jobsjob_idstepsshell

다만 위 GitHub Actions의 공식 문서를 보면, 우분투 환경에서도 시스템 기본인 dash 대신 'bash -e {0}'를 사용하도록 설정되어 있어 '==' 사용에도 문제가 없을 것으로 보입니다.
저는 개인적으로 조금 더 넓은 호환성을 생각해서 =를 선호하곤 하지만, 여기서는 무엇을 선택하시든 괜찮을 것 같습니다.
읽어주셔서 감사합니다!

image

done

test $result = "PASS" # if non-zero, fail
test $result = "PASS" # if non-zero, fail
Copy link
Contributor

Choose a reason for hiding this comment

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

마지막에 빈 공백라인 추가


# Check if the file is in the exception list
for exception in "${EXCEPTION_FILES[@]}"; do
if [ "$f" = "$exception" ]; then
result_for_f="PASS"
Copy link
Contributor

Choose a reason for hiding this comment

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

미사용

# Check if file is listed in CMakeLists.txt
local filename
filename=$(basename "$f")
if ! grep -qE "/$filename\$" cubrid/CMakeLists.txt; then
Copy link
Contributor

Choose a reason for hiding this comment

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

cubrid/CMakeLists.txt 파일에 존재하지 않는 경우는 어디서 체크되나요?

@H2SU H2SU marked this pull request as draft January 3, 2025 02:14
@H2SU H2SU marked this pull request as ready for review January 6, 2025 07:36
@H2SU
Copy link
Contributor Author

H2SU commented Jan 6, 2025

문제가 되는 부분만 수정하여 반영하였습니다.
기존 로직에서 이상한 점이 있어 해당 부분은 따로 처리하려합니다.

@H2SU H2SU requested review from vimkim and hornetmj January 6, 2025 07:39
@hornetmj hornetmj merged commit 3f492f9 into CUBRID:develop Jan 6, 2025
9 of 10 checks passed
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.

5 participants