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

Provide migration hook #5046

Merged
merged 48 commits into from
Sep 19, 2023
Merged

Provide migration hook #5046

merged 48 commits into from
Sep 19, 2023

Conversation

yhabteab
Copy link
Member

@yhabteab yhabteab commented Jun 21, 2023

For testing this hook you can use these PRs from Reporting and X509. But make sure you have created some report schedules with the master branch beforehand.

You can also use this example hook to test PHP code migrations:

diff --git a/library/Icinga/Application/ApplicationBootstrap.php b/library/Icinga/Application/ApplicationBootstrap.php
index 4e7ddd954..b909aa20e 100644
--- a/library/Icinga/Application/ApplicationBootstrap.php
+++ b/library/Icinga/Application/ApplicationBootstrap.php
@@ -7,6 +7,7 @@ use DirectoryIterator;
 use ErrorException;
 use Exception;
 use Icinga\Application\ProvidedHook\DBMigration;
+use Icinga\Application\ProvidedHook\DummyMigrator;
 use ipl\I18n\GettextTranslator;
 use ipl\I18n\StaticTranslator;
 use LogicException;
@@ -770,6 +771,7 @@ abstract class ApplicationBootstrap
     protected function registerApplicationHooks(): self
     {
         Hook::register('migration', DBMigration::class, DBMigration::class);
+        Hook::register('migration', DummyMigrator::class, DummyMigrator::class);
 
         return $this;
     }
<?php

namespace Icinga\Application\ProvidedHook;

use Icinga\Application\Hook\PHPMigrationHook;

class DummyMigrator extends PHPMigrationHook
{
    protected const EXECUTE_AFTER_SCHEMA_VERSION = '2.12.0';

    public function getDescription(): string
    {
        return $this->translate('This is Icinga Web 2');
    }

    public function getName(): string
    {
        return $this->translate('Icinga Web 2');
    }

    public function apply(): bool
    {
        return $this->run(static::VERSION, function () {
            var_dump("Dummy");
        });
    }
}

resolves #5043

@cla-bot cla-bot bot added the cla/signed label Jun 21, 2023
@yhabteab yhabteab force-pushed the provide-migration-hook branch 9 times, most recently from 6b9292a to c49e303 Compare June 26, 2023 09:41
@yhabteab yhabteab requested review from lippserd and nilmerg June 26, 2023 10:01
@yhabteab yhabteab force-pushed the provide-migration-hook branch 4 times, most recently from 9091615 to 01f2f69 Compare July 24, 2023 12:04
@yhabteab yhabteab requested a review from flourish86 July 24, 2023 12:18
@yhabteab yhabteab marked this pull request as ready for review July 24, 2023 12:22
@yhabteab yhabteab force-pushed the provide-migration-hook branch from e82ad07 to 7c78594 Compare July 24, 2023 15:37
Copy link
Contributor

@flourish86 flourish86 left a comment

Choose a reason for hiding this comment

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

Collapse Toggle Icons

Screenshot 2023-07-25 at 10 47 27

Please invert the arrow direction. We use. icons to reflect actions not states. So, caret-down when collapsed and vice versa.

List Item "Migrate" Button

Screenshot 2023-07-25 at 10 49 41
Please apply the style from the mockups

Disabled style

The buttons still look clickable, even when they are disabled.

Apply All Button

"Apply All" is too generic. Please provide a clearer label, like "Migrate all modules".
Screenshot 2023-07-25 at 10 50 09

Visual aligment

Please align on baseline
Screenshot 2023-07-25 at 10 52 47

Collapse state won't be conserved on auto-refresh

Collapse button position

While it makes sense to have the collapse button stay in place for interaction reasons (button doesn't "jump"), clarity suffers from a structural aspect, when the migration list is expanded. Please move the button to the bottom of the list.

Collapse button width

Screenshot 2023-07-25 at 11 07 49

The button doesn't need span the full width. It's doesn't make a difference at first, but when focussed it looks awkward. Probably display: inline-block will easily fix this.

@yhabteab
Copy link
Member Author

List Item "Migrate" Button

Screenshot 2023-07-25 at 10 49 41
Please apply the style from the mockups

The mockups uses the style of an action/button link but this is a regular submit button and ist styled like another submit buttons of Icinga Web 2. So, why should we change it here?

While it makes sense to have the collapse button stay in place for interaction reasons (button doesn't "jump"), clarity suffers from a structural aspect, when the migration list is expanded. Please move the button to the bottom of the list.

A details/summary tag is standardised to always render the details after the collapse/expand marker. But if you insist on changing this behavior, you can try, cause I'm not and wasn't able to do this.

@yhabteab
Copy link
Member Author

For reference here're the mockups!
Pending Migrations

Migrations List

@yhabteab yhabteab force-pushed the provide-migration-hook branch from 7c78594 to 438f7aa Compare July 26, 2023 12:04
@yhabteab
Copy link
Member Author

List Item "Migrate" Button

Screenshot 2023-07-25 at 10 49 41
Please apply the style from the mockups

The mockups uses the style of an action/button link but this is a regular submit button and ist styled like another submit buttons of Icinga Web 2. So, why should we change it here?

Done!

@yhabteab yhabteab requested a review from flourish86 July 26, 2023 12:55
@yhabteab yhabteab force-pushed the provide-migration-hook branch from 796bc49 to f54df07 Compare July 26, 2023 14:23
Copy link
Contributor

@flourish86 flourish86 left a comment

Choose a reason for hiding this comment

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

Screenshot 2023-07-26 at 16 23 14

Not sure about the reason, but your branch messes with the Icinga DB list items

Screenshot 2023-07-26 at 16 26 47

  1. I find it acceptable to label to the list item Button "Apply" since we're talking about a list of (pending) migrations, though it differs from the mockups.

Following this reasoning it doesn't make sense to label the "global" button this way, though. "Apply All Modules" just doesn't make sense, since you aren't list modules.
"Apply All Module Migrations" would be correct. I would rather go for "Apply all", though.

  1. Please remove the button background according to the mockups. It's less about default styles than visual clarity in this case and the button shape makes the element too distracting from the headline and the content.

@flourish86
Copy link
Contributor

List Item "Migrate" Button

Screenshot 2023-07-25 at 10 49 41
Please apply the style from the mockups

The mockups uses the style of an action/button link but this is a regular submit button and ist styled like another submit buttons of Icinga Web 2. So, why should we change it here?

Done!

Your point makes actually sense, sometimes I'm living too much in Icinga Db Web world. 😅 Maybe we should address this globally for icingaweb2 in a different context.

@yhabteab yhabteab removed request for lippserd and nilmerg July 26, 2023 15:31
yhabteab and others added 23 commits September 19, 2023 14:37
@nilmerg nilmerg force-pushed the provide-migration-hook branch from 0a57b35 to 9c6d930 Compare September 19, 2023 12:38
@nilmerg nilmerg dismissed stale reviews from sukhwinder33445 and flourish86 September 19, 2023 12:38

outdated

@nilmerg nilmerg added area/framework Affects third party integration/development area/modules Affects module support/integration labels Sep 19, 2023
@nilmerg nilmerg merged commit e4c9266 into master Sep 19, 2023
@nilmerg nilmerg deleted the provide-migration-hook branch September 19, 2023 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/framework Affects third party integration/development area/modules Affects module support/integration cla/signed enhancement New feature or improvement
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Provide a way to easily perform database migrations
5 participants