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

TACODEV-774: enable metric on argo series #6

Merged
merged 1 commit into from
Oct 8, 2021
Merged

TACODEV-774: enable metric on argo series #6

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions argocd-install/values-override.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,3 +64,9 @@ server:
kind: '*'
orphanedResources:
warn: false
metrics:
enabled: true
dex.metrics.enabled: true
Copy link
Contributor

@Jaesang Jaesang Oct 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

위 metrics.enabled처럼

dex:
  metrics:
    enabled: true

와 같이

통일을 하는게 좋아보입니당

Copy link
Contributor Author

@intelliguy intelliguy Oct 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

https://github.com/openinfradev/decapod-base-yaml/blob/main/lma/base/site-values.yaml 와 같이 기본적으로
'aaa.aaa.aaa: value' 형태로 valueoverride를 하고 있었습니다.
위에는 블록에서 헛갈릴수 있을꺼 같아서 반영했습니다.
제안하신 형태로 수정한다면 (원 파일의) 아래에 정의된 값들도 모두 바꿔야 하고 한줄이 여러줄로 변경되어 훨씬 긴 파일을 유지해야합니다.
추가로 yaml에서 인덴트가 정확하게 보이지 않아서 개인적으로는 선호하지 않습니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

decapod site-values.yaml에서는 그렇게 하는게 룰이니 말씀하신게 맞습니다.
이 파일에서는 위 server: 처럼 그 형태를 같이 하는게 맞지않을까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

앞에 밝힌 이유로 더 좋은 표현방식으로 보이지는 않습니다.
또한 valueoverride를 위한 값의 정의이기도 하고요.
이 표현방법에 대해서는 논의를 해보시지요.
건별 선택보다는 룰을 정해서 가는게 좋겠습니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

2 가지 표현 형식이 모두 가능하더라도 단일 파일 내에서는 하나의 표현 방식으로 가는게 바람직해 보입니다.

Copy link
Contributor

@robertchoi80 robertchoi80 Oct 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

좀 애매한 케이스네요. 지금 형태는 일관성이 없어보이긴 해서 사실 통일된 형태로 작성하는데 맞긴 한데,, 만약 단일 값 override 때문에 세 라인씩 인덴트 넣어가며 작성하는게 큰 오버헤드처럼 느껴진다면, 중재안으로 ## 같은 주석으로 섹션 구분하는 느낌으로 처리하는 건 어떨까요?? 그냥 즉흥적인 아이디어라서, 동의하는 분이 많지 않으면 그냥 전부 통일하는 방향으로 가구요.

server:
  key1:
     key2
      ...

######################################
# Simple one-line variable overrides #
######################################
xx.enabled: true
yy.enabled: true
...

Copy link
Contributor Author

@intelliguy intelliguy Oct 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

논의는 https://github.com/openinfradev/tks-issues/issues/30 에서 히스토리가 남을수 있게 진행하시는게 좋아보입니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

이건 닫히면 찾기 힘들어서

controller.metrics.enabled: true
repoServer.metrics.enabled: true
redis-ha.haproxy.metrics.enabled: false