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

[ISSUE-008] Github Actions 적용 #25

Merged
merged 5 commits into from
May 28, 2022
Merged

Conversation

hoyahozz
Copy link
Member

@hoyahozz hoyahozz commented May 27, 2022

ISSUE


작업 내용

  • Github Actions 를 적용, PR 시 빌드 테스트 진행
  • app 모듈, presentation 모듈의 테스트 모듈 삭제
  • presentation 모듈에 Junit 라이브러리 추가

실행 화면

Screen Shot Screen Shot

Check List

  • PR 제목은 [TICKET-{N}] PR 제목으로 작성
  • CI/CD 통과 여부
  • 1명 이상의 Approve 확인 후 Merge
  • 관련 이슈 연결

@hoyahozz hoyahozz added this to the 프로젝트 구축 milestone May 27, 2022
@hoyahozz hoyahozz self-assigned this May 27, 2022
@hoyahozz hoyahozz changed the title Feature/issue 008 GitHub action [ISSUE-008] Github Actions 적용 May 27, 2022
const val kotlinCoroutine = "1.5.1"
const val retrofit = "2.7.1"
const val okhttp = "4.3.1"
const val kotlinCoroutineVersion = "1.5.1"
Copy link
Member

Choose a reason for hiding this comment

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

이거 버전이랑 이름이 따로 되어 있는 게 조금 불편한 것 같아서 버전이랑 dependency 따로 분리해서 관리하지 말고 하나로 합쳐서 관리하는 거에 대해서 다들 어떻게 생각하시나요?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

아 이 부분 뭔가 불편해서 뭐가 문제지 싶어서 저 의견을 말씀드렸던 건데 불편한 점 찾았어요
버전 따로 적어두는 건 지금처럼 해도 좋을 것 같습니다!
DependencyInfo 쪽에서 이름이랑 버전 두 개 파라미터로 보내서 넣어주고 있는 걸 지금 봤네요 ㅠㅠ 죄송합니다.. 🥲

  1. 상수는 관례적으로 대문자로 작성하고 있는 것으로 알고 있어서 KOTLIN_COROUTINE_VERSION 이런 식으로 변경하는 것이 좋을 것 같습니다(이 문제는 지금 제가 작업 중인 부분에도 관련이 있어서 지금 PR은 그냥 두시고 제가 일괄 수정해 놓을게요!)
  2. 추가로 Versions 안에 있는 상수들인데 굳이 변수 어미로 Version을 중복해서 붙여주지 않아도 좋지 않을까 하는 사소한 의견입니다!
    Versions.kotlinCoroutineVersion <- 이렇게 봤을 때 Version이라는 정보가 중복으로 들어가면서 변수명이 의미 없이 길어진 느낌이라서요! (요 부분도 괜찮으시면 제가 힐트 추가하면서 같이 수정할게요!)
  3. 공식 문서에서 전부 코루틴을 Coroutines로 s를 붙여서 작성하고 있는데 저희도,, 코루틴에 s붙여서 변수 이름 정하는 게 어떨까용?

세 가지 제안에 대해서 괜찮으시면 제가 수정해서 이대로 PR 올리겠습니다

Copy link
Member Author

Choose a reason for hiding this comment

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

넹! 다 괜찮은 것 같아요! 👍

@jihee-dev jihee-dev self-requested a review May 28, 2022 05:23
Copy link
Member

@jihee-dev jihee-dev left a comment

Choose a reason for hiding this comment

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

고생하셨습니다!

@hoyahozz hoyahozz merged commit b5f80c5 into develop May 28, 2022
@hoyahozz hoyahozz deleted the feature/issue-008-github-action branch May 28, 2022 05:24
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.

[프로젝트 구축] Github Actions
2 participants