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

Library split #747

Merged
merged 42 commits into from
May 27, 2024
Merged

Library split #747

merged 42 commits into from
May 27, 2024

Conversation

lippserd
Copy link
Member

@lippserd lippserd commented Apr 19, 2024

Closes #654

@cla-bot cla-bot bot added the cla/signed label Apr 19, 2024
@lippserd lippserd force-pushed the library-split-2 branch 5 times, most recently from e044278 to 6e065b9 Compare April 26, 2024 08:15
@julianbrost
Copy link
Contributor

Commit 14f2f84 does more than just moving code around. It seems to completely replace the ConvertCamelCase() function with something different. It also touches a lot of db struct field tags where I first thought that might have accidentally added to the comment but it looks too consistent for that to actually be the case. Was this necessary to handle changed behavior of the new implementation? But if that's the case, why was the implementation changed?

@lippserd
Copy link
Member Author

Commit 14f2f84 does more than just moving code around. It seems to completely replace the ConvertCamelCase() function with something different. It also touches a lot of db struct field tags where I first thought that might have accidentally added to the comment but it looks too consistent for that to actually be the case. Was this necessary to handle changed behavior of the new implementation? But if that's the case, why was the implementation changed?

The previous implementation was quite opinionated, e.g. without taking numbers into account. For our library, I have opted for a general approach.

pkg/icingadb/db.go Outdated Show resolved Hide resolved
@lippserd lippserd force-pushed the library-split-2 branch 2 times, most recently from e4319e4 to 0f585f7 Compare April 30, 2024 11:49
pkg/config/config.go Outdated Show resolved Hide resolved
pkg/config/config.go Outdated Show resolved Hide resolved
pkg/logging/logging.go Outdated Show resolved Hide resolved
pkg/types/unix_milli.go Outdated Show resolved Hide resolved
pkg/types/unix_milli_test.go Outdated Show resolved Hide resolved
pkg/icingadb/history/sla.go Outdated Show resolved Hide resolved
pkg/strcase/strcase.go Outdated Show resolved Hide resolved
pkg/strcase/strcase.go Outdated Show resolved Hide resolved
@lippserd lippserd force-pushed the library-split-2 branch 2 times, most recently from af4d8d1 to b1bc41e Compare May 7, 2024 19:44
pkg/strcase/strcase.go Outdated Show resolved Hide resolved
Copy link
Contributor

@julianbrost julianbrost left a comment

Choose a reason for hiding this comment

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

Just a few small things I noticed. I think we can start doing the actual move soon. What's the plan here? There seems to be no clear pattern like pkg/ will be moved as a whole, is each sub-package planned to either be moved or kept here (hence also the one "is this supposed to be moved?" question)?

pkg/database/db.go Outdated Show resolved Hide resolved
pkg/database/cleanup.go Outdated Show resolved Hide resolved
pkg/strcase/strcase_test.go Outdated Show resolved Hide resolved
pkg/types/unix_milli.go Outdated Show resolved Hide resolved
Copy link
Contributor

@julianbrost julianbrost left a comment

Choose a reason for hiding this comment

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

Looks fine for me now. (Not formally clicking approve here as you wanted to still clean up the commit history. And maybe even do the switchover to the library in this PR?)

internal/config/config.go Show resolved Hide resolved
pkg/redis/client.go Outdated Show resolved Hide resolved
pkg/database/db.go Outdated Show resolved Hide resolved
internal/config/config.go Show resolved Hide resolved
cmd/icingadb/main.go Outdated Show resolved Hide resolved
Copy link
Contributor

@julianbrost julianbrost left a comment

Choose a reason for hiding this comment

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

Now same level of approval from my side as in #747 (review):

Looks fine for me now. (Not formally clicking approve here as you wanted to still clean up the commit history. And maybe even do the switchover to the library in this PR?)

@lippserd
Copy link
Member Author

I cleaned up the commit history and switched to using icinga-go-library in this PR.
Note that the new lib is here at the moment. Once you've approved, I'll force the push to main and tag it as version 1.0.0 and change go.mod here accordingly.

@yhabteab
Copy link
Member

Looks like something went wrong with the rebasing? 1ddaa70, 1d9dc74 and 563045c seem to appear twice in the commit history.

@yhabteab
Copy link
Member

Looks like something went wrong with the rebasing? 1ddaa70, 1d9dc74 and 563045c seem to appear twice in the commit history.

Ohh 😧! Never mind! It's GitHub's fault!

internal/config/config.go Show resolved Hide resolved
cmd/icingadb/main.go Outdated Show resolved Hide resolved
pkg/config/config.go Outdated Show resolved Hide resolved
Copy link
Contributor

@julianbrost julianbrost left a comment

Choose a reason for hiding this comment

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

Fine for me now. At least on the "does not seem to break anything" level. And I wouldn't go into more clean this up side-quests here unless it fixes something that's worse than the current state in Icinga DB. This PR is huge and cumbersome to work with already.

Once you've approved, I'll force the push to main and tag it as version 1.0.0 and change go.mod here accordingly.

If in doubt, it could also be tagged as v0.1.0, then do some more few PRs in the new repo and then make it a 1.0.

lippserd and others added 4 commits May 23, 2024 12:42
`ColumnMap` provides a cached mapping of structs exported fields to
their database column names. By default, all exported struct fields are
mapped to their database column names using snake case notation. The `-`
(hyphen) directive for the db tag can be used to exclude certain fields.
Since `ColumnMap` uses cache, the returned slice MUST NOT be modified
directly.
@lippserd
Copy link
Member Author

Once you've approved, I'll force the push to main and tag it as version 1.0.0 and change go.mod here accordingly.

If in doubt, it could also be tagged as v0.1.0, then do some more few PRs in the new repo and then make it a 1.0.

Yeah, I started with v0.1.0.

@lippserd lippserd marked this pull request as ready for review May 24, 2024 07:57
Copy link
Member

@yhabteab yhabteab left a comment

Choose a reason for hiding this comment

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

I found one last thing in the new library that should be changed, otherwise this LFTM!

@julianbrost
Copy link
Contributor

Not really relevant for Icinga DB, will be fixed and become a v0.2.0.

@julianbrost julianbrost merged commit 35c6b3f into main May 27, 2024
31 checks passed
@julianbrost julianbrost deleted the library-split-2 branch May 27, 2024 08:08
@oxzi oxzi added this to the 1.2.1 milestone Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants