-
Notifications
You must be signed in to change notification settings - Fork 0
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
[FEAT/#6] pr-checker 설정 #7
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pr-checker 설정하시느라 고생 많으셨습니다!!
야무진 리드. 👍
.github/workflows/pr_checker.yml
Outdated
- name: Access Local Properties | ||
env: | ||
HFM_BASE_URL: ${{ secrets.BASE_URL }} | ||
TEST_BASE_URL: ${{ secrets.TEST_BASE_URL }} | ||
run: | | ||
echo base.url=\"$BASE_URL\" >> local.properties | ||
echo test.base.url=\"$TEST_BASE_URL\" >> local.properties |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 로컬 프로퍼티에 base.url
과 test.base.url
두개가 저장되는 것으로 보여요.
슬랙에 공유해주신 것은 test.base.url
하나인 것 같은데 공유가 누락된걸까요????
혹은 추후 prod와 dev를 분리하기 위해 미리 만들어둔건가용?
한번 확인해주시면 감사하겠습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
base.url 는 prod 이고 test.base.url 는 dev 전용인가요?! 맞다면 조금 더 직관적인 이름을 가지고 가는게 어떨까 싶습니다..!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
일단 현재는 test.base.url만 사용하기 때문에 슬랙에 공유드린 그대로 로컬프로퍼티에 적용해주시면 됩니다!!
말씀하신대로 추후에 서버를 분리하게 되었을 때 ci 코드를 수정하고 싶지 않아서 미리 넣어두었는데 생각해보니 어차피 릴리즈 전에 추가해야 할 내용들이 많아서 그냥 지금 현재 사용하는 부분들만 넣어두는 것이 맞는 것 같아요!!
그리고 더 직관적인 네이밍으로 수정하는 것 너무너무 동의합니다!! test는 dev로 이름 수정할게요~~!!
app/build.gradle.kts
Outdated
buildConfigField( | ||
"String", | ||
"BASE_URL", | ||
properties.getProperty("test.base.url") | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제가 알기로는 이 코드가 debug에만 있으면 release를 상정한 깃허브 액션에서 오류가 발생하는 걸로 알고있어요. 그래서 이를 해결하기 위해 default로 옮긴 것 같아요!
debug
와 relase
모두 BASE_URL을 만드는 코드를 작성해두는건 어떨까요??
사실 앱잼 끝나고 스프린터 더 진행 후 릴리즈 예정이라 그때 해도 전혀전혀 문제 없긴 합니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
하 저의 삽질 이유가 정확히 이거였는데요🥹 일단 서버를 분리하지는 않았으니까 default로 두고 추후에 서버 분리 되었을 때 함께 수정하도록 할게요!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고 많으셨습니다!! 우리 리드 항상 고생 많아용,,,최고 방어 다음에 먹으러갑니다 ㅠㅠ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다!! 항상 고생 많으십니다. 다음에 한번 더 방어 꼭 먹으러가요ㅠㅠ
.github/workflows/pr_checker.yml
Outdated
- name: Access Local Properties | ||
env: | ||
HFM_BASE_URL: ${{ secrets.BASE_URL }} | ||
TEST_BASE_URL: ${{ secrets.TEST_BASE_URL }} | ||
run: | | ||
echo base.url=\"$BASE_URL\" >> local.properties | ||
echo test.base.url=\"$TEST_BASE_URL\" >> local.properties |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
base.url 는 prod 이고 test.base.url 는 dev 전용인가요?! 맞다면 조금 더 직관적인 이름을 가지고 가는게 어떨까 싶습니다..!
.github/workflows/pr_checker.yml
Outdated
- name: Change gradlew permissions | ||
run: chmod +x ./gradlew | ||
|
||
- name: Grant execute permission for gradlew | ||
run: chmod +x gradlew |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: 이 두가지 step은 동일한 작업을 수행하는 것 같은데 맞을까요? ./
는 현재 디렉토리를 의미하는걸로 알고 있습니다. 그래서 중복된 step이라면 제거하는게 좋아 보여요:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 인정합니다 이거 왜 2개 들어가있지ㅠㅠ 찾아주셔서 감사해요!!
.github/workflows/pr_checker.yml
Outdated
- name: Access Local Properties | ||
env: | ||
HFM_BASE_URL: ${{ secrets.BASE_URL }} | ||
TEST_BASE_URL: ${{ secrets.TEST_BASE_URL }} | ||
run: | | ||
echo base.url=\"$BASE_URL\" >> local.properties | ||
echo test.base.url=\"$TEST_BASE_URL\" >> local.properties |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: 파일에 URL을 설정하는 부분에서 환경변수를 잘못 참조하고 있는걸로 보여요
env:
에서 HFM_BASE_URL
로 환경변수를 설정했는데, echo 명령어에서는 $BASE_URL
로 사용하고 있네요.
- name: Access Local Properties | |
env: | |
HFM_BASE_URL: ${{ secrets.BASE_URL }} | |
TEST_BASE_URL: ${{ secrets.TEST_BASE_URL }} | |
run: | | |
echo base.url=\"$BASE_URL\" >> local.properties | |
echo test.base.url=\"$TEST_BASE_URL\" >> local.properties | |
- name: Access Local Properties | |
env: | |
HFM_BASE_URL: ${{ secrets.BASE_URL }} | |
TEST_BASE_URL: ${{ secrets.TEST_BASE_URL }} | |
run: | | |
echo base.url=\"$HFM_BASE_URL\" >> local.properties | |
echo test.base.url=\"$TEST_BASE_URL\" >> local.properties |
이렇게 바꾸면 오류 없이 적용 될 것 같습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
효빈님 기초세팅 고생 많았어요ㅠ 야무진 리드와 함께 행복 앱잼 합시다!
Related issue 🛠
Work Description ✏️
Screenshot 📸
N/A
Uncompleted Tasks 😅
N/A
To Reviewers 📢
CI 세팅했습니다! PR 날릴 때마다 자동으로 실행될거에요!!
그리고 린트 체크도 추가했으니 앞으로 커밋 날리기 전에 린트 체크 꼭 하고 커밋해주세요
리뷰어도 세팅해놔서 PR 날릴 때 자동으로 추가될건데 혹시 안되면 저한테 말씀해주세요..!!