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

bugfix loading config & test code #20

Merged
merged 12 commits into from
Apr 5, 2023
Merged

bugfix loading config & test code #20

merged 12 commits into from
Apr 5, 2023

Conversation

jmnote
Copy link
Contributor

@jmnote jmnote commented Apr 4, 2023

What this PR does / why we need it (변경 내용 / 필요성):

현재 코드는 config load 단계에서 panic이 발생하는 버그가 있습니다. 포인터만 생성하고 실제 메모리 할당 과정이 없이 Unmarshal하기 때문인 것으로 보입니다. Unmarshal 전에 메모리를 할당해주어 panic을 방지합니다.

그리고 config_test.go에 TestLoad() 테스트 코드를 작성하였습니다.

Which issue(s) this PR fixes (관련 이슈):

#19 에 대응되며
#16 과도 관련이 있습니다.

Special notes for your reviewer (리뷰어에게 하고 싶은 말):

리뷰 감사합니다.

Unmarshal 전에 메모리를 할당해줍시다.
https://stackoverflow.com/questions/20478577/why-does-json-unmarshal-work-with-reference-but-not-pointer

Makefile는 checks(go fmt, vet, staticcheck, golangci-lint, test)가 병렬로 수행되던 것을 순차실행하기 위해 변경했습니다. (병렬 수행하면 어느 단계에서 메시지가 발생했는지 확인이 어려움)

pkg/handler/dashboard.go 는 변경한 것이 없는데 자동 교정되었어요.

Additional documentation, usage docs, etc. (기타 관련 문서, 사용법 등):

없음

@jmnote jmnote linked an issue Apr 4, 2023 that may be closed by this pull request
@jmnote jmnote requested a review from dozer-jang April 4, 2023 13:51
@jmnote jmnote self-assigned this Apr 4, 2023
@jmnote jmnote added the bug Something isn't working label Apr 4, 2023
@kuoss kuoss deleted a comment from codecov-commenter Apr 4, 2023
@codecov-commenter
Copy link

codecov-commenter commented Apr 4, 2023

Codecov Report

Patch coverage: 57.69% and project coverage change: +14.13 🎉

Comparison is base (0d1474d) 20.00% compared to head (c6f9757) 34.13%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@             Coverage Diff             @@
##             main      #20       +/-   ##
===========================================
+ Coverage   20.00%   34.13%   +14.13%     
===========================================
  Files           6        6               
  Lines         250      249        -1     
===========================================
+ Hits           50       85       +35     
+ Misses        190      145       -45     
- Partials       10       19        +9     
Impacted Files Coverage Δ
pkg/store/userStore.go 0.00% <0.00%> (ø)
pkg/store/alertRuleStore.go 59.09% <59.09%> (ø)
pkg/configuration/config.go 55.00% <100.00%> (+47.50%) ⬆️

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@dozer-jang dozer-jang left a comment

Choose a reason for hiding this comment

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

말씀해주신 Unmarshal 에러는 loadYaml 함수 호출부분에서 & operator를 붙이지 않아서 발생한 문제입니다.

그리고 포인터 타입의 변수를 초기화할 때
var userConf *userConfig = &UserConfig{} 보다는
userConfig := &UserConfig{} or var userConf = &UserConfig{} 이 간결할 것 같습니다.

pkg/configuration/config.go Outdated Show resolved Hide resolved
pkg/configuration/config.go Outdated Show resolved Hide resolved
pkg/configuration/config_test.go Outdated Show resolved Hide resolved
pkg/store/alertrulestore.go Outdated Show resolved Hide resolved
@pull-request-size pull-request-size bot added size/L and removed size/M labels Apr 4, 2023
@jmnote
Copy link
Contributor Author

jmnote commented Apr 4, 2023

alertRuleStore는 관련해서는 그냥 &만 붙여서 되는 게 아니고, 구조적인 문제가 있었네요. 각 파일이 groups 를 가질 수 있고, 여러 개의 설정 파일이 있을 수 있긴 때문에 최종적으로 groupsList를 가지고 있어야 합니다. 전체적으로 groupsList를 적용하고 테스트코드도 추가했습니다.

@dozer-jang
Copy link
Contributor

alertRuleStore는 관련해서는 그냥 &만 붙여서 되는 게 아니고, 구조적인 문제가 있었네요. 각 파일이 groups 를 가질 수 있고, 여러 개의 설정 파일이 있을 수 있긴 때문에 최종적으로 groupsList를 가지고 있어야 합니다. 전체적으로 groupsList를 적용하고 테스트코드도 추가했습니다.

네 확인했습니다. 하나의 파일이 여러 group을 가질 수 있는 구조였네요. 감사합니다

@dozer-jang
Copy link
Contributor

alertrRuleStore.go 파일을 camel case로 변경하셨어요. golang에서 파일명에대한 컨벤션이 정해져있는 건 아니지만 전통적으로 표준 라이브러리들이 all lowercase로 작성되었고 venti도 all lowercase 이었기 때문에 기존 형식으로 변경 부탁드립니다.

golang/go#36060 (comment)

@dozer-jang
Copy link
Contributor

dozer-jang commented Apr 5, 2023

최초로 DatasourceConfig 필드의 Datasource 와 Service Discovery로 발견되는 Datasource가 동시에 쓰이고 있어서 포인터 타입으로 선언했던 것으로 기억합니다. 근데 말씀하신대로 생각해보니 너무 많은 포인터가 쓰이고 있네요.

type Config struct {
	Version           string
	UserConfig        UsersConfig
	DatasourcesConfig DatasourcesConfig
}

...
        var userConf UsersConfig
	err = loadConfig(userConfigFile, &userConf)
	if err != nil {
		return nil, fmt.Errorf("error on loading User Config: %w", err)
        }

Config타입의 UserConfig 필드의 타입을 value타입으로 변경하면 좋을 것 같습니다.

Service Discovery 이후에 DatasourcesDatasourceStore에 따로 저장하고 있습니다.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@jmnote
Copy link
Contributor Author

jmnote commented Apr 5, 2023

alertrRuleStore.go 파일을 camel case로 변경하셨어요. golang에서 파일명에대한 컨벤션이 정해져있는 건 아니지만 전통적으로 표준 라이브러리들이 all lowercase로 작성되었고 venti도 all lowercase 이었기 때문에 기존 형식으로 변경 부탁드립니다.

golang/go#36060 (comment)

네, 좋은 지적입니다. helloworld.go, hello_world.go 이렇게 2가지 스타일로 작성하는 관례를 확인했습니다. 소문자로 작성하되 필요시 _를 사용해도 된다. 이 정도인 것 같네요. 우리 프로젝트에서는 type AlertRuleStore struct를 담은 파일의 경우 alert_rule_store.go 와 같은 형식을 사용하도록 일관적인 규칙을 부여해도 좋을 것 같습니다. 다만 이번 PR은 이미 사이즈가 크고, 전체 go파일을 일관성 있게 적용하는 것이 중요하므로 별도 issue에서 논의 후 처리하는 게 좋겠습니다.

#21 이슈로 등록했습니다.

@kuoss kuoss deleted a comment from dozer-jang Apr 5, 2023
@jmnote
Copy link
Contributor Author

jmnote commented Apr 5, 2023

최초로 DatasourceConfig 필드의 Datasource 와 Service Discovery로 발견되는 Datasource가 동시에 쓰이고 있어서 포인터 타입으로 선언했던 것으로 기억합니다. 근데 말씀하신대로 생각해보니 너무 많은 포인터가 쓰이고 있네요.

type Config struct {
	Version           string
	UserConfig        UsersConfig
	DatasourcesConfig DatasourcesConfig
}

...
        var userConf UsersConfig
	err = loadConfig(userConfigFile, &userConf)
	if err != nil {
		return nil, fmt.Errorf("error on loading User Config: %w", err)
        }

Config타입의 UserConfig 필드의 타입을 value타입으로 변경하면 좋을 것 같습니다.

Service Discovery 이후에 DatasourcesDatasourceStore에 따로 저장하고 있습니다.

수정했습니다. ( 너무 길어지는 것 같아서 ping comment는 삭제했습니다. ㅎㅎ )

Copy link
Contributor

@dozer-jang dozer-jang left a comment

Choose a reason for hiding this comment

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

코드 확인했습니다. 의견 받아주셔서 정말 감사드립니다.
Config 타입의 DatasourceConfig도 value 타입으로 변경하면 좋을 것 같습니다. 하지만 datasourceStore 코드도 영향을 받으니 따로 이슈로 수정하는 것이 좋겠습니다.

다시 한번 감사드리고 approval합니다.

@jmnote
Copy link
Contributor Author

jmnote commented Apr 5, 2023

긴 내용 장시간 리뷰하느라 수고하셨습니다. 감사합니다. merge합니다.

@jmnote jmnote merged commit 14cea77 into main Apr 5, 2023
@jmnote jmnote deleted the 19-config-load-panic branch April 5, 2023 03:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

config load panic
3 participants