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

Flexible ID tags #105

Merged
merged 1 commit into from
Nov 30, 2023
Merged

Flexible ID tags #105

merged 1 commit into from
Nov 30, 2023

Conversation

sukhwinder33445
Copy link
Contributor

@sukhwinder33445 sukhwinder33445 commented Oct 4, 2023

resolves #85

  • change upgrade schema file name

@cla-bot cla-bot bot added the cla/signed CLA is signed by all contributors of a PR label Oct 4, 2023
@sukhwinder33445 sukhwinder33445 force-pushed the introduce-object-id-tags branch 2 times, most recently from cf0fcc1 to 3193abf Compare October 5, 2023 10:50
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.

The schema upgrade script will probably have to be renamed as well before merging the PR. But as far as I can tell, the corresponding changes that will be needed in notifications-web were not started yet, were they? (So I don't want to say it will be the next one and should use 016.sql yet.)

schema/pgsql/upgrades/018.sql Outdated Show resolved Hide resolved
schema/pgsql/upgrades/018.sql Outdated Show resolved Hide resolved
@julianbrost
Copy link
Collaborator

Changes look good so far from just looking over the code. I'll continue with testing once a corresponding web PR is ready (so that I don't break my test setup while doing so).

@julianbrost
Copy link
Collaborator

julianbrost commented Oct 9, 2023

@yhabteab Can you do me a favor and test this, especially in conjunction with Icinga/icinga-notifications-web#133?

Also, in case anyone needs it, I already have a SQL snippet lying around to roll back that schema change:

ALTER TABLE object ADD COLUMN host text, ADD COLUMN service text;
UPDATE object SET host = (SELECT value FROM object_id_tag WHERE object_id = id AND tag = 'host');
UPDATE object SET service = (SELECT value FROM object_id_tag WHERE object_id = id AND tag = 'service');
ALTER TABLE object ALTER COLUMN host SET NOT NULL;
DROP TABLE object_id_tag;

internal/object/object.go Outdated Show resolved Hide resolved
internal/object/object.go Outdated Show resolved Hide resolved
@sukhwinder33445 sukhwinder33445 force-pushed the introduce-object-id-tags branch 2 times, most recently from 56163c6 to 808e216 Compare October 24, 2023 13:41
yhabteab
yhabteab previously approved these changes Oct 24, 2023
@julianbrost julianbrost dismissed their stale review October 26, 2023 10:43

Changes requested were done, the Go part looks fine so far but I'm waiting on the PHP part for a final review

@sukhwinder33445 sukhwinder33445 force-pushed the introduce-object-id-tags branch 2 times, most recently from 1b2ec0e to 1f660e9 Compare November 22, 2023 14:05
@julianbrost julianbrost merged commit f4123ad into main Nov 30, 2023
3 checks passed
@julianbrost julianbrost deleted the introduce-object-id-tags branch November 30, 2023 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla/signed CLA is signed by all contributors of a PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow flexible ID tags
3 participants