icingadb icon indicating copy to clipboard operation
icingadb copied to clipboard

Published 20 hours ago • verified publisher icon Icinga

Library split

Open lippserd opened this issue 10 months ago • 2 comments

Closes #654

lippserd avatar Apr 19 '24 13:04 lippserd

Commit 14f2f848bafa8cc27d43462c209366a222d91791 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?

julianbrost avatar Apr 29 '24 15:04 julianbrost

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.

lippserd avatar Apr 30 '24 07:04 lippserd

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?)

Same! I have nothing to complain about anymore!

yhabteab avatar May 17 '24 13:05 yhabteab

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.

lippserd avatar May 22 '24 10:05 lippserd

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

yhabteab avatar May 22 '24 13:05 yhabteab

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!

yhabteab avatar May 22 '24 13:05 yhabteab

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 avatar May 24 '24 07:05 lippserd

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

julianbrost avatar May 27 '24 08:05 julianbrost