-
Notifications
You must be signed in to change notification settings - Fork 9
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
TACODEV-774: enable metric on argo series #6
Conversation
ba32117
to
c0fd8c3
Compare
c0fd8c3
to
03267a5
Compare
@@ -64,3 +64,9 @@ server: | |||
kind: '*' | |||
orphanedResources: | |||
warn: false | |||
metrics: | |||
enabled: true | |||
dex.metrics.enabled: true |
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.
위 metrics.enabled처럼
dex:
metrics:
enabled: true
와 같이
통일을 하는게 좋아보입니당
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.
https://github.com/openinfradev/decapod-base-yaml/blob/main/lma/base/site-values.yaml 와 같이 기본적으로
'aaa.aaa.aaa: value' 형태로 valueoverride를 하고 있었습니다.
위에는 블록에서 헛갈릴수 있을꺼 같아서 반영했습니다.
제안하신 형태로 수정한다면 (원 파일의) 아래에 정의된 값들도 모두 바꿔야 하고 한줄이 여러줄로 변경되어 훨씬 긴 파일을 유지해야합니다.
추가로 yaml에서 인덴트가 정확하게 보이지 않아서 개인적으로는 선호하지 않습니다.
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.
decapod site-values.yaml에서는 그렇게 하는게 룰이니 말씀하신게 맞습니다.
이 파일에서는 위 server:
처럼 그 형태를 같이 하는게 맞지않을까요?
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.
앞에 밝힌 이유로 더 좋은 표현방식으로 보이지는 않습니다.
또한 valueoverride를 위한 값의 정의이기도 하고요.
이 표현방법에 대해서는 논의를 해보시지요.
건별 선택보다는 룰을 정해서 가는게 좋겠습니다.
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 가지 표현 형식이 모두 가능하더라도 단일 파일 내에서는 하나의 표현 방식으로 가는게 바람직해 보입니다.
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.
좀 애매한 케이스네요. 지금 형태는 일관성이 없어보이긴 해서 사실 통일된 형태로 작성하는데 맞긴 한데,, 만약 단일 값 override 때문에 세 라인씩 인덴트 넣어가며 작성하는게 큰 오버헤드처럼 느껴진다면, 중재안으로 ## 같은 주석으로 섹션 구분하는 느낌으로 처리하는 건 어떨까요?? 그냥 즉흥적인 아이디어라서, 동의하는 분이 많지 않으면 그냥 전부 통일하는 방향으로 가구요.
server:
key1:
key2
...
######################################
# Simple one-line variable overrides #
######################################
xx.enabled: true
yy.enabled: true
...
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.
논의는 https://github.com/openinfradev/tks-issues/issues/30 에서 히스토리가 남을수 있게 진행하시는게 좋아보입니다.
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.
이건 닫히면 찾기 힘들어서
@@ -64,3 +64,9 @@ server: | |||
kind: '*' | |||
orphanedResources: | |||
warn: false | |||
metrics: | |||
enabled: true | |||
dex.metrics.enabled: true |
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 가지 표현 형식이 모두 가능하더라도 단일 파일 내에서는 하나의 표현 방식으로 가는게 바람직해 보입니다.
reverted merge