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

fix: Troubleshooting failure to get information when an application has multiple sources #15

Merged
merged 3 commits into from
Oct 3, 2024
Merged

fix: Troubleshooting failure to get information when an application has multiple sources #15

merged 3 commits into from
Oct 3, 2024

Conversation

eogns47
Copy link

@eogns47 eogns47 commented Oct 1, 2024 •
edited
Loading

related issue #10

❓AS-IS: Failure to get information when an application has multiple sources

📝TO-BE: Change code that handled only a single source to handle arrays

🤔REASON: No alerts at all for multiple sources. This is a serious bug

↔️Changes: Change the files in your catalog template to handle multi-source

@eogns47 eogns47 requested a review from daengdaengLee October 1, 2024 18:06
@eogns47 eogns47 self-assigned this Oct 1, 2024
daengdaengLee
daengdaengLee reviewed Oct 3, 2024
View reviewed changes
Copy link
Member

@daengdaengLee daengdaengLee left a comment

Choose a reason for hiding this comment

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

케이스가

  1. source.repoURL 이 존재할 때
  2. source.repoURL 이 없고 sources 배열이 있을 때
    이렇게 두 가지인 것 같아요.
    템플릿에 이 두 가지를 분기칠 수 있는 방법이 있을까요?

@eogns47
Copy link
Author

eogns47 commented Oct 3, 2024

케이스가

  1. source.repoURL 이 존재할 때
  2. source.repoURL 이 없고 sources 배열이 있을 때
    이렇게 두 가지인 것 같아요.
    템플릿에 이 두 가지를 분기칠 수 있는 방법이 있을까요?

아하.. 그렇네요 다시 생각해보고 커밋 올리겠습니다

@daengdaengLee
Copy link
Member

엇 @eogns47 님 생각해보니

  1. if spec.source 가 있을 땐 spec.source.repoURL 사용
  2. else if spec.sources 가 있을 땐 range spec.sources -> .repoURL 사용
  3. else 고정 문자열로 그냥 "no repoURL" 같은 거 사용

이런 식으로 하면 더 안전할 것 같습니다!

@eogns47
Copy link
Author

eogns47 commented Oct 3, 2024

@daengdaengLee 넵 한번 작성해보겠습니다!

@eogns47 eogns47 force-pushed the issue-19766-manjoo branch from 0a9ab64 to 2953ad9 Compare October 3, 2024 04:35
@eogns47 eogns47 force-pushed the issue-19766-manjoo branch from 2953ad9 to 029bc17 Compare October 3, 2024 04:41
@eogns47 eogns47 requested a review from daengdaengLee October 3, 2024 04:41
daengdaengLee
daengdaengLee reviewed Oct 3, 2024
View reviewed changes
Copy link
Member

@daengdaengLee daengdaengLee left a comment

Choose a reason for hiding this comment

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

notifications_catalog/install.yaml 파일에서 | 나 |- 를 사용해서 멀티 라인 문자열을 읽기 쉽게 표현하는 부분은 그대로 유지되면 좋을 것 같아요. 혹시 뭔가 포멧터같은 게 잘못 돈 걸까요?

notifications_catalog/templates/app-deployed.yaml Outdated
@@ -44,7 +53,7 @@ teams:
},
{
"name": "Repository",
"value": "{{.app.spec.source.repoURL}}"
"value": "{{range .app.spec.sources}}{{.repoURL}} {{end}}"
Copy link
Member

Choose a reason for hiding this comment

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

여기도 처리해야 하지 않을까요?

notifications_catalog/templates/app-health-degraded.yaml Outdated
@@ -40,7 +49,7 @@ teams:
},
{
"name": "Repository",
"value": "{{.app.spec.source.repoURL}}"
"value": "{{range .app.spec.sources}}{{.repoURL}} {{end}}"
Copy link
Member

Choose a reason for hiding this comment

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

여기도 처리해야 하지 않을까요?

notifications_catalog/templates/app-sync-failed.yaml Outdated
@@ -44,7 +53,7 @@ teams:
},
{
"name": "Repository",
"value": "{{.app.spec.source.repoURL}}"
"value": "{{range .app.spec.sources}}{{.repoURL}} {{end}}"
Copy link
Member

Choose a reason for hiding this comment

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

여기도 처리해야 하지 않을까요?

notifications_catalog/templates/app-sync-running.yaml Outdated
@@ -43,7 +52,7 @@ teams:
},
{
"name": "Repository",
"value": "{{.app.spec.source.repoURL}}"
"value": "{{range .app.spec.sources}}{{.repoURL}} {{end}}"
Copy link
Member

Choose a reason for hiding this comment

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

여기도 처리해야 하지 않을까요?

@eogns47
Copy link
Author

eogns47 commented Oct 3, 2024

notifications_catalog/install.yaml 파일에서 | 나 |- 를 사용해서 멀티 라인 문자열을 읽기 쉽게 표현하는 부분은 그대로 유지되면 좋을 것 같아요. 혹시 뭔가 포멧터같은 게 잘못 돈 걸까요?

수동으로 일일이 바꾸느라 빠트린게 있었네요 😅 수정하겠습니다!

@eogns47
Copy link
Author

eogns47 commented Oct 3, 2024

@daengdaengLee 빠트린 부분 수정 완료했습니다 !

daengdaengLee
daengdaengLee approved these changes Oct 3, 2024
View reviewed changes
Copy link
Member

@daengdaengLee daengdaengLee left a comment

Choose a reason for hiding this comment

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

LGTM!

@eogns47 eogns47 merged commit c9a1572 into issue-19766 Oct 3, 2024
3 checks passed
@eogns47 eogns47 deleted the issue-19766-manjoo branch October 3, 2024 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daengdaengLee daengdaengLee daengdaengLee approved these changes

Assignees

@eogns47 eogns47

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@eogns47 @daengdaengLee