-
Notifications
You must be signed in to change notification settings - Fork 0
Import Icinga Notifications internal/utils
#127
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
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
oxzi
added a commit
to Icinga/icinga-notifications
that referenced
this pull request
Apr 28, 2025
With Icinga/icinga-go-library#127 being merged, the internal/utils package will be moved to the IGL and can be removed here. This is another step to unify our Go codebase.
7fc3dce
to
944fc74
Compare
Additional changes after a note from @lippserd:
Furthermore, I have removed the two commits adding |
ee2d0e0
to
05bc544
Compare
05bc544
to
fa35896
Compare
oxzi
added a commit
to Icinga/icinga-notifications
that referenced
this pull request
Apr 29, 2025
With Icinga/icinga-go-library#64 and Icinga/icinga-go-library#127 being merged, the internal/utils package is moved to the IGL and can be removed here. This is another step to unify our Go codebase.
lippserd
requested changes
Apr 30, 2025
fa35896
to
c8349a9
Compare
c8349a9
to
67cd647
Compare
oxzi
added a commit
to Icinga/icinga-notifications
that referenced
this pull request
Apr 30, 2025
With Icinga/icinga-go-library#64 and Icinga/icinga-go-library#127 being merged, the internal/utils package is moved to the IGL and can be removed here. This is another step to unify our Go codebase.
61f2a44
to
ec0f388
Compare
lippserd
requested changes
May 7, 2025
This change was the result of investigating Go's new rangefunc experiment[0]. The utilization of this novel language feature - which can also be indirectly used in the absence of `GOEXPERIMENT=rangefunc` - ensures that the map is traversed in key order. [0]: https://go.dev/wiki/RangefuncExperiment Imported from Icinga/icinga-notifications@e371553
Require Go 1.23 Imported from Icinga/icinga-notifications@ac235b7
Doing so allows multiple columns to be excluded without breaking the API compatibility. Recently added slices functions even shortens the code.
The MakeString function got another variadic parameter, allowing multiple transformers against the newly created String. As one example, the newly introduced TransformEmptyStringToNull function can be used, mimicking Icinga Notifications' ToDBString behavior. To replace Icinga Notifications' ToDBInt utility function, a similar MakeInt function and TransformZeroIntToNull was introduced. Inspired by: - ToDBInt, Icinga/icinga-notifications@f0b39f0 - ToDBString, Icinga/icinga-notifications@8183344
ec0f388
to
41e23b6
Compare
lippserd
approved these changes
May 7, 2025
oxzi
added a commit
to Icinga/icinga-notifications
that referenced
this pull request
May 8, 2025
With Icinga/icinga-go-library#64 and Icinga/icinga-go-library#127 being merged, most of the internal/utils package is moved to the IGL and can be removed here. This is another step to unify our Go codebase.
I have successfully tested Icinga Notifications with this changes in Icinga/icinga-notifications#310 and a |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR imports the
internal/utils
package from Icinga Notifications intoutils
for general purpose functions and intodatabase
for database-related utility functions. The commit history was kept, but messages were slightly altered since the original messages often referred changes outside theinternal/utils
package.My motivation for this merge was to use a utility function already implemented, but residing in the wrong project's utility package. Since the IGL is the place for common code, I would like to get rid of utility packages outside this repository.
Steps performed to create this PR
Create Patches
Within the
icinga-notifications
repository.Apply Patches
Within this repository.
First, create a
githook(5)
to verify each applied patch works.Plot twist: each patch will fail now.
Start applying patches:
$ git am -3 --directory utils/ ../icinga-notifications/*.patch
Each patch now requires some manual intervention, e.g., making it compile or manually move some code to
./database/util.go
.If the current patch works, build it manually once and continue afterwards.
$ git add utils/ database/ && git am --continue
The
0011-Switch-to-icinga-go-library.patch
patch was skipped since it does not made sense in this context.Verifying Methods
To make sure that nothing was lost, I have manually diffed each function.
No diff for
ToDBString
,ToDBInt
, andIterateOrderedMap
.Reword Commit Messages
Since most commit messages referred a certain change and the changes in the
util.go
file was a drive-by change, I have reworded each commit message and linked the original commit on GitHub.Cleanup
As further specified in #127 (comment), some functions and commits were dropped again.