fix: Correctly escape consecutive commas in cleanSetParameters #11
Conversation
util/helm/cmd.go
Outdated
| @@ -352,6 +352,7 @@ func cleanSetParameters(val string) string { | |||
| if strings.HasPrefix(val, `{`) && strings.HasSuffix(val, `}`) { | |||
| return val | |||
| } | |||
| re := regexp.MustCompile(`,`) | |||
There was a problem hiding this comment.
여기가 아니라 위에 기존 re 를 수정해야 할 것 같아요. (346 번 라인)
이렇게 하면 모든 , 가 잡히는데 기존 re 에서 의도한 것처럼 /, 인 경우는 빼고 잡아내야 할 것 같습니다.
기존 코드의 경우
앞에 `/` 가 아닌 다른 문자가 있는 `,` 문자
를 잡아내고 있어요.
문제가 되는 부분은
- 앞에 아무 문자도 없는 경우일 때
,,처럼,가 연속 2번 나올 때 뒤에 나오는,
이 두 경우가 기존 정규식으로 안 잡혀요.
기존 의도는 유지한 채로 저 두 경우까지 잡아내는 방법을 고민해보면 좋을 것 같습니다.
@mirageoasis 님도 같이 봐주세요~
There was a problem hiding this comment.
근데 왜 slash가 아닌 다른 문자가 있는 ','를 잡아야하나요?
파일 경로 때문에 그런건가요?
There was a problem hiding this comment.
/, 는 이미 이스케이프 처리 된 상태라서 저것까지 잡으면 /, 가 //, 가 되어버려서요.
이미 이스케이프 처리된 건 건너뛰려고 하는 것 같아요.
There was a problem hiding this comment.
unescapedRe := regexp.MustCompile(`([^\\]),`)
return unescapedRe.ReplaceAllString(val, `$1\\,`)이런식은 어떤가요?
Signed-off-by: daengdaengLee <[email protected]>
|
@eogns47, @mirageoasis 님 안녕하세요.
감사합니다. |
|
@daengdaengLee 넵 그렇게 해보겠습니다. 감사합니다! |
|
@daengdaengLee 혹시 코드가 조금 길어지긴하지만 아예 정규식을 사용하지 않는 방식은 어떨까요?? |
| re := regexp.MustCompile(`,`) | ||
| return re.ReplaceAllString(val, `$1\,`) | ||
|
|
||
| var result strings.Builder |
There was a problem hiding this comment.
개인적으로 추가된 코드가 가독성을 크게 해치지 않고 재사용할 여지가 없는 것 같아서 굳이 분리하지 않았습니다..!
혹시 상기 이유 외에 필요 여부가 더 있다면 분리해보겠습니다!
There was a problem hiding this comment.
테스트 케이스는 helm_test.go 의 TestHelmArgCleaner 에 추가해주시면 될 것 같습니다.
|
@eogns47 님 안녕하세요. 저는 코드 로직 괜찮다고 생각합니다. 감사합니다. |
Signed-off-by: KangManJoo <[email protected]>
수정 후 커밋하였습니다! |
util/helm/cmd.go
Outdated
| var result strings.Builder | ||
| for _, r := range val { | ||
| if r == ',' { | ||
| if result.Len() == 0 || result.String()[result.Len()-1] != '\\' { |
There was a problem hiding this comment.
- 위에
_로 처리한 게 index 니까 저걸i로 잡아서i == 0으로 처리해도 될 것 같아요. - 직전
r을 별도 변수값으로 관리해서 (ex.prevR) 그거랑 비교하는게 좋을 것 같아요. 이렇게 하면 매번 문자열 빌드해서요.
There was a problem hiding this comment.
아래는 예시입니다.
var result strings.Builder
var prevR rune
for i, r := range test {
if r == ',' {
if i == 0 || prevR != '\\' {
result.WriteString(`\,`)
} else {
result.WriteRune(r)
}
} else {
result.WriteRune(r)
}
prevR = r
}
return result.String()There was a problem hiding this comment.
@daengdaengLee 다시 커밋해봤습니다! 혹시 '매번 문자열을 빌드한다'가 result.String()함수를 호출하면서 객체를 생성하는 것을 말씀하시는게 맞을까요?
Signed-off-by: KangManJoo <[email protected]>
related issue #9 (1 problem / 2 problems)
AS-IS: Consecutive commas are not properly escaped in helm template parameters.
TO-BE: Consecutive commas will be correctly escaped to ensure they are handled appropriately in the helm template parameters.
REASON: Properly escaping consecutive commas is essential to avoid issues with parameter parsing. This change improves the handling of parameters and aligns with the expected behavior when using helm, ensuring that parameters with multiple commas are treated correctly.
Changes: Added test code and fixed errors.
Checklist: