Skip to content

Add column external_uuid to contact/contactgroup/channel table #216

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Oct 31, 2025
Merged

Add column external_uuid to contact/contactgroup/channel table #216

merged 3 commits into from
Oct 31, 2025

Conversation

@sukhwinder33445
Copy link
Contributor

@sukhwinder33445 sukhwinder33445 commented Jun 11, 2024 •
edited by julianbrost
Loading

resolves #192

Draft until the following PR is merged:

  • blocked by: Feature/http api new approach icinga-notifications-web#348 (This schema change is solely needed for Notifications Web, so no need to merge this earlier than the corresponding PR there. On the other hand, the schema change breaks Notifications Web without that PR as is couldn't insert rows anymore as it doesn't yet know the required column.)

@sukhwinder33445 sukhwinder33445 self-assigned this Jun 11, 2024
@cla-bot cla-bot bot added the cla/signed CLA is signed by all contributors of a PR label Jun 11, 2024
@sukhwinder33445 sukhwinder33445 requested a review from nilmerg June 11, 2024 08:29
julianbrost
julianbrost requested changes Jun 11, 2024
View reviewed changes
@sukhwinder33445 sukhwinder33445 force-pushed the add-column-external-uuid branch from 767820c to 0a3cc97 Compare June 11, 2024 09:53
julianbrost
julianbrost requested changes Jun 11, 2024
View reviewed changes
Copy link
Collaborator

@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.

You're missing the change in the full schema.sql file (looks like I've totally missed this before).

Also, if the NOT NULL constraints remain,

  1. a value should be set for existing rows.
  2. this should not be merged without Icinga/icinga-notifications-web#199, otherwise this would break creating contacts.

@sukhwinder33445 sukhwinder33445 force-pushed the add-column-external-uuid branch 5 times, most recently from b997f95 to bcc0bf0 Compare June 12, 2024 12:31
@sukhwinder33445 sukhwinder33445 mentioned this pull request Jun 12, 2024
Closed
raviks789
raviks789 reviewed Jun 12, 2024
View reviewed changes
@sukhwinder33445 sukhwinder33445 force-pushed the add-column-external-uuid branch from bcc0bf0 to 2813f1e Compare June 13, 2024 07:34
@sukhwinder33445 sukhwinder33445 force-pushed the add-column-external-uuid branch from 2813f1e to c630f50 Compare June 27, 2024 13:23
julianbrost
julianbrost reviewed Jun 27, 2024
View reviewed changes
Copy link
Collaborator

@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.

General question: do you (= team web) expect/want this to be merged soon?

schema/pgsql/schema.sql Outdated
Comment on lines 60 to 65
CONSTRAINT pk_contact PRIMARY KEY (id),
UNIQUE (username)
UNIQUE (username),
UNIQUE (external_uuid),
CONSTRAINT pk_contact PRIMARY KEY (id)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm wondering why you reordered the constraints here. Also, in the rest of the schema file, so far the primary key is given first, I'd keep it that way (it also makes sense to me, one can say it's the most important constraint).

@sukhwinder33445 sukhwinder33445 force-pushed the add-column-external-uuid branch from c630f50 to ebe647d Compare June 27, 2024 19:49
@sukhwinder33445
Copy link
Contributor Author

This can wait, so far it is a stand-alone change without any dependencies to other branches (except the linked web branch). When the corresponding web branch is ready to be merged, this should be merged together with it.

@nilmerg nilmerg force-pushed the add-column-external-uuid branch from ebe647d to 8f8e51c Compare August 26, 2025 09:15
@nilmerg
Copy link
Member

nilmerg commented Aug 26, 2025

I've only rebased and fixed the conflicts.

@Jan-Schuppik Jan-Schuppik mentioned this pull request Sep 3, 2025
Merged
@nilmerg nilmerg force-pushed the add-column-external-uuid branch from eb223eb to ed33127 Compare October 22, 2025 12:06
@nilmerg nilmerg requested a review from julianbrost October 22, 2025 12:10
@nilmerg
Copy link
Member

nilmerg commented Oct 22, 2025

Added mysql changes. Should be somewhat final now.

@julianbrost
Copy link
Collaborator

So this is in scope for the next release?

Should be somewhat final now.

blocked by: Icinga/icinga-notifications-web#348

Is "somewhat finial" final enough that we could merge it any time? Or should we still wait for anything in that PR?

@julianbrost julianbrost added this to the 0.2.0 milestone Oct 28, 2025
@nilmerg
Copy link
Member

nilmerg commented Oct 28, 2025

this should not be merged without Icinga/icinga-notifications-web#199 Icinga/icinga-notifications-web#348, otherwise this would break creating contacts.

still applies

@julianbrost julianbrost mentioned this pull request Oct 29, 2025
Draft
julianbrost
julianbrost reviewed Oct 29, 2025
View reviewed changes
@julianbrost
Copy link
Collaborator

FYI: With these changes, the PR should also make it into a "ready to merge anytime the corresponding Notifications Web PR is ready", similar to #344 (review).

@sukhwinder33445 sukhwinder33445 force-pushed the add-column-external-uuid branch from ed33127 to bf27922 Compare October 29, 2025 13:34
@sukhwinder33445 sukhwinder33445 changed the title Add column external_uuid to contact/contactgroup table Add column external_uuid to contact/contactgroup/channel table Oct 29, 2025
@nilmerg @sukhwinder33445
mysql: Introduce external_uuid to contact, contactgroup and `ch…
9edddb5 
…annel`

The `UUID()` function exists since MySQL 8.0 and is available in
MariaDB 10.2.2. Both current minimum requirements for Icinga DB.
@sukhwinder33445 sukhwinder33445 force-pushed the add-column-external-uuid branch from bf27922 to 9edddb5 Compare October 29, 2025 13:41
@julianbrost julianbrost marked this pull request as draft October 29, 2025 15:40
julianbrost
julianbrost approved these changes Oct 29, 2025
View reviewed changes
Copy link
Collaborator

@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.

This PR is fine and can be merged any time the corresponding Notifications Web PR is ready. Though it probably shouldn't merged earlier as the added columns are required without a default, hence without the corresponding change in Notifications Web, older versions can't insert new rows anymore.

Thus, I've converted the PR to a draft for the moment (so there's a hint that it maybe shouldn't be merged immediately despite this approval) with a corresponding note in the PR description.

@nilmerg nilmerg marked this pull request as ready for review October 31, 2025 14:56
@nilmerg nilmerg merged commit 4d26c72 into main Oct 31, 2025
26 checks passed
@nilmerg nilmerg deleted the add-column-external-uuid branch October 31, 2025 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@raviks789 raviks789 raviks789 left review comments

@julianbrost julianbrost julianbrost approved these changes

@nilmerg nilmerg Awaiting requested review from nilmerg

Assignees

@sukhwinder33445 sukhwinder33445

Labels

cla/signed CLA is signed by all contributors of a PR

Projects

None yet

Milestone

0.2.0

Development

Successfully merging this pull request may close these issues.

Add column external_uuid to contact and contactgroup

5 participants

@sukhwinder33445 @nilmerg @julianbrost @raviks789