Library split by lippserd · Pull Request #747 · Icinga/icingadb

lippserd · 2024-04-19T13:44:30Z

Closes #654

julianbrost · 2024-04-29T15:21:19Z

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 · 2024-04-30T07:01:07Z

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

pkg/config/config.go

pkg/logging/logging.go

pkg/types/unix_milli.go

pkg/types/unix_milli_test.go

pkg/icingadb/history/sla.go

pkg/strcase/strcase.go

julianbrost

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

pkg/database/cleanup.go

pkg/strcase/strcase_test.go

pkg/types/unix_milli.go

julianbrost

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/database.go

internal/config/config.go

pkg/redis/client.go

pkg/database/db.go

internal/config/config.go

cmd/icingadb/main.go

julianbrost

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 · 2024-05-22T10:05:03Z

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 · 2024-05-22T13:46:08Z

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

yhabteab · 2024-05-22T13:49:47Z

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

cmd/icingadb/main.go

pkg/config/config.go

…ed argument

julianbrost

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.

…ssed argument

`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 · 2024-05-24T07:57:31Z

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.

yhabteab

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

julianbrost · 2024-05-27T08:08:02Z

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

cla-bot bot added the cla/signed label Apr 19, 2024

lippserd force-pushed the library-split-2 branch 5 times, most recently from e044278 to 6e065b9 Compare April 26, 2024 08:15

julianbrost reviewed Apr 30, 2024

View reviewed changes

pkg/icingadb/db.go Outdated Show resolved Hide resolved

lippserd force-pushed the library-split-2 branch 2 times, most recently from e4319e4 to 0f585f7 Compare April 30, 2024 11:49

julianbrost reviewed Apr 30, 2024

View reviewed changes

oxzi mentioned this pull request Apr 30, 2024

utils.FromUnixMilli has floating point rounding issues #753

Closed

julianbrost reviewed May 2, 2024

View reviewed changes

pkg/strcase/strcase.go Outdated Show resolved Hide resolved

lippserd force-pushed the library-split-2 branch from 0f585f7 to ea8365b Compare May 2, 2024 11:38

julianbrost reviewed May 2, 2024

View reviewed changes

pkg/strcase/strcase.go Outdated Show resolved Hide resolved

lippserd force-pushed the library-split-2 branch 2 times, most recently from af4d8d1 to b1bc41e Compare May 7, 2024 19:44

julianbrost requested changes May 8, 2024

View reviewed changes

pkg/strcase/strcase.go Outdated Show resolved Hide resolved

lippserd force-pushed the library-split-2 branch from c34ca2c to 0e39b96 Compare May 8, 2024 09:39

lippserd requested a review from julianbrost May 8, 2024 09:41

julianbrost requested changes May 13, 2024

View reviewed changes

pkg/database/db.go Outdated Show resolved Hide resolved

pkg/database/cleanup.go Show resolved Hide resolved

pkg/strcase/strcase_test.go Outdated Show resolved Hide resolved

pkg/types/unix_milli.go Outdated Show resolved Hide resolved

julianbrost requested a review from yhabteab May 13, 2024 15:03

lippserd requested a review from julianbrost May 14, 2024 09:30

julianbrost reviewed May 14, 2024

View reviewed changes

yhabteab reviewed May 14, 2024

View reviewed changes

internal/config/database.go Outdated Show resolved Hide resolved

yhabteab reviewed May 16, 2024

View reviewed changes

internal/config/config.go Show resolved Hide resolved

pkg/redis/client.go Outdated Show resolved Hide resolved

julianbrost requested changes May 17, 2024

View reviewed changes

pkg/database/db.go Outdated Show resolved Hide resolved

internal/config/config.go Show resolved Hide resolved

julianbrost reviewed May 17, 2024

View reviewed changes

cmd/icingadb/main.go Outdated Show resolved Hide resolved

julianbrost reviewed May 17, 2024

View reviewed changes

Refine types#UnixMilli tests

1e221e4

lippserd force-pushed the library-split-2 branch from 444a7f0 to b72e218 Compare May 22, 2024 09:52

lippserd requested review from julianbrost and yhabteab May 22, 2024 10:05

yhabteab reviewed May 22, 2024

View reviewed changes

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

lippserd added 2 commits May 23, 2024 09:46

Replace utils.Fatal() with utils.PrintErrorThenExit()

957e610

config#ParseFlags(): Parse result into value pointed to by the pass…

9f83544

…ed argument

julianbrost reviewed May 23, 2024

View reviewed changes

lippserd and others added 4 commits May 23, 2024 12:42

config#FromYAMLFile(): Parse result into value pointed to by the pa…

b396a36

…ssed argument

Introduce GetAddr() for database#DB and redis#Client

a73940e

Allow to dynamically define type constraint name

a016cc2

lippserd force-pushed the library-split-2 branch from b72e218 to 11cef36 Compare May 23, 2024 11:27

lippserd added 2 commits May 24, 2024 09:56

Use icinga-go-library

7c068d4

Remove library code

be4b450

lippserd force-pushed the library-split-2 branch from 11cef36 to be4b450 Compare May 24, 2024 07:56

lippserd requested review from julianbrost and yhabteab May 24, 2024 07:57

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

julianbrost approved these changes May 24, 2024

View reviewed changes

yhabteab approved these changes May 27, 2024

View reviewed changes

julianbrost merged commit 35c6b3f into main May 27, 2024

julianbrost deleted the library-split-2 branch May 27, 2024 08:08

oxzi added this to the 1.2.1 milestone Jul 30, 2024

lippserd mentioned this pull request Sep 20, 2024

config/: enforce type constraints via generics, not reflection Icinga/icinga-go-library#72

Merged

Conversation

lippserd commented Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

julianbrost commented Apr 29, 2024

Uh oh!

lippserd commented Apr 30, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

julianbrost left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

julianbrost left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

julianbrost left a comment

Choose a reason for hiding this comment

Uh oh!

lippserd commented May 22, 2024

Uh oh!

yhabteab commented May 22, 2024

Uh oh!

yhabteab commented May 22, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

julianbrost left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lippserd commented May 24, 2024

Uh oh!

yhabteab left a comment

Choose a reason for hiding this comment

Uh oh!

julianbrost commented May 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lippserd commented Apr 19, 2024 •

edited

Loading

julianbrost left a comment •

edited

Loading