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

feat(view): cluster 요약에 release tag 추가(#361) #724

Merged
merged 3 commits into from
Sep 29, 2024

Conversation

pithesun
Copy link
Contributor

Related issue

#361

Result

cluster에 해당하는 release tag를 확인할 수 있습니다.
한 클러스에 여러개의 release tag가 있다면 가장 최신의 태그를 노출합니다.

image

Work list

(/w. @rakseong)

✅ Commit 타입에 tag와 releaseTag, Cluster 타입에 latestReleaseTag를 추가하였습니다.

✅ 클러스터의 commitNodeList에서 releaseTag들을 뽑아와서, 가장 최근의 releaseTag를 Summary에 노출하였습니다.

Discussion

Summary.util.ts 파일 구조상 코드의 가독성이 떨어진다고 판단
-> #723 에서 리팩토링을 진행예정입니다.

ytaek
ytaek previously approved these changes Sep 14, 2024
Copy link
Contributor

@ytaek ytaek left a comment

Choose a reason for hiding this comment

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

드!!!
디!!!
어!!!!

tag 가 보이는 군요 👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍
🥇🥇🥇🥇🥇🥇🥇🥇🥇🥇🥇🥇🥇🥇🥇🥇🥇🥇🥇🥇🥇🥇🥇🥇🥇🥇🥇🥇🥇🥇🥇🥇🥇🥇🥇🥇

해당 PR을 보니 test case들도 여러가지 넣어볼 것들 많을 것 같습니다
이것들 포함해서 커멘트 달린 부분 검토해주시고, 유효한 것들은 이슈로 등록 부탁드리겠습니다!

Comment on lines +49 to +50
tags: [],
releaseTags: [],
Copy link
Contributor

Choose a reason for hiding this comment

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

TC(test case)까지 신경쓰시는 세심함!!! 😉

tag 값들도 실제로 넣어서 Test Assertion (Expect) 하는 부분도 추가해주시면 좋을 것 같습니다!!!
(잊지 않도록 해당 테스트 케이스 추가 건 이슈로 등록해주시면 감사하겠습니다!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#725 으로 추가하였습니다😎

@@ -91,6 +91,7 @@ const Summary = () => {
));
})}
</div>
<div>{cluster.latestReleaseTag}</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

추후에 더 이쁘게 꾸며주시는거죠? 😈😈😈😈😈
중요 tag 같은 경우들은 chip 이나 badge 써도 될 것 같네요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😉얘도 이슈로 추가하겠습니당 #726

@@ -19,6 +19,7 @@ export type Summary = {
export type Cluster = {
clusterId: number;
summary: Summary;
latestReleaseTag: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

음. 해당 클러스터에 포함된 release tag는 전부 가지고 있고,
보여줄 때만 latest를 보여줘야 하지 않을까요?

해당 부분도 검토해주시고, 이 부분도 issue로 등록 부탁드리겠습니다!

Copy link
Contributor Author

@pithesun pithesun Sep 18, 2024

Choose a reason for hiding this comment

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

#727 로 등록했습니다.

Comment on lines +74 to +76
"message": "Added node.js.yml",
"tags": ["v1.0.0,"],
"releaseTags": ["v1.0.0,"]
Copy link
Contributor

Choose a reason for hiding this comment

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

👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍

추가로 tag가 여러개인 경우,
release tag 이외인 케이스도 추후 등록하면 좋겠습니다.

(이것도 이슈로.. ㅎㅎ)

return latestTagIdx;
}, 0);

const latestTag = tags[latestTagIndex];
Copy link
Contributor

Choose a reason for hiding this comment

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

(궁금) return tags[latestTagIndex]를 하지 않고 변수로 latestTag를 선언해 주신 이유가 있을까요?

xxxjinn
xxxjinn previously approved these changes Sep 21, 2024
Copy link
Contributor

@xxxjinn xxxjinn left a comment

Choose a reason for hiding this comment

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

LGTM!!!!!!!✨✨✨✨

@ytaek
Copy link
Contributor

ytaek commented Sep 27, 2024

@pithesun 님, 이슈 반영 되셨으면, merge 해주셔도 좋을 것 같습니다!

@taboowiths taboowiths added this to the v0.7.2 milestone Sep 27, 2024
@pithesun
Copy link
Contributor Author

@pithesun 님, 이슈 반영 되셨으면, merge 해주셔도 좋을 것 같습니다!

충돌해결하면서 포스푸시하는 바람에 리뷰가 날라갔네요🥲
번거롭지만 한 번 더 체크 부탁드립니다


let prevTagNumber = 0;
const latestTagIndex = validTags.reduce((latestTagIdx, tag, idx) => {
const currnetTagNumber = tagToNumber(tag, maxTagLength);
Copy link
Contributor

Choose a reason for hiding this comment

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

생각해보니 태그를 number로 만들어서 비교하지 않고도
sort를 사용해서 가장 최신 태그를 확인해볼수도 있겠네요!!

Copy link
Contributor

Choose a reason for hiding this comment

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

생각해보니 태그를 number로 만들어서 비교하지 않고도 sort를 사용해서 가장 최신 태그를 확인해볼수도 있겠네요!!

음. string으로 sort하는거 말씀이시라면

v0.2.3 이랑
v0.02.2 이랑 ordering이 달라집니다 ㅎㅎ

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

@ytaek ytaek left a comment

Choose a reason for hiding this comment

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

LGTM!!!

@pithesun pithesun merged commit 87154d2 into githru:main Sep 29, 2024
2 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