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

VIP Integrations: Improves reading of config files and add multiple configs support of same integration #5643

Open
wants to merge 24 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
f415cae
🔨 REFACTOR: reading of VIP Integrations config
mehmoodak Jun 12, 2024
c89c999
ADDS: Support for handling multiple configs of the integration
mehmoodak Jun 13, 2024
c962f96
Merge branch 'develop' into cafe-919/update-IntegrationVipConfig-clas…
mehmoodak Jun 13, 2024
107649d
🔨 REFACTOR: reduce return statements
mehmoodak Jun 13, 2024
3d654bb
Merge branch 'develop' into cafe-919/update-IntegrationVipConfig-clas…
mehmoodak Jun 13, 2024
8b2916a
🔨 REFACTOR: incorporate suggestion of merging if statements
mehmoodak Jun 13, 2024
9d0ee40
➕ ADDS: tests for VIP_Integrations_Config_Test
mehmoodak Jun 14, 2024
a47bee0
💫 UPDATE: VIP_Integrations_Test
mehmoodak Jun 14, 2024
d77380b
💫 UPDATE: VIP_Integration_Test
mehmoodak Jun 14, 2024
13fdecc
💫 UPDATE: VIP_Integration_Test to include tests of IntegrationVipConf…
mehmoodak Jun 14, 2024
0da775d
➖ REMOVE: removed unused field
mehmoodak Jun 14, 2024
accfa94
Merge branch 'develop' into cafe-919/update-IntegrationVipConfig-clas…
mehmoodak Jun 14, 2024
de0b802
👌 IMPROVE: test name
mehmoodak Jun 14, 2024
a83322d
🐛 FIX: test activate_platform_integrations_are_activating_integration…
mehmoodak Jun 14, 2024
4653616
👌 IMPROVE: VIP_Integrations_Config_Test
mehmoodak Jun 16, 2024
ae48d59
➕ ADDS: multi config integrations test
mehmoodak Jun 16, 2024
6133b8e
Merge branch 'develop' into cafe-919/update-IntegrationVipConfig-clas…
mehmoodak Jun 16, 2024
f575c06
👌 IMPROVE: comments
mehmoodak Jun 17, 2024
251976d
👌 IMPROVE: variable name
mehmoodak Jun 20, 2024
5ba7e30
Merge branch 'develop' into cafe-919/update-IntegrationVipConfig-clas…
mehmoodak Jun 20, 2024
da24f72
Merge branch 'develop' into cafe-919/update-IntegrationVipConfig-clas…
mehmoodak Aug 15, 2024
d36f954
👌 IMPROVE: PHPDocs
mehmoodak Aug 15, 2024
45da1ae
🐛 FIX: sonar cloud issue
mehmoodak Aug 15, 2024
fe14e06
🐛 FIX: ConfigMap example
mehmoodak Aug 22, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
186 changes: 0 additions & 186 deletions integrations/integration-vip-config.php
Copy link
Member Author

Choose a reason for hiding this comment

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

  • Changed the name to vip-integrations-config.php and refactored the code.

This file was deleted.

145 changes: 132 additions & 13 deletions integrations/integration.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@

namespace Automattic\VIP\Integrations;

use Automattic\VIP\Integrations\IntegrationVipConfig;
use Org_Integration_Status;
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we're actually moving away from differing the statuses and want it all in one.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes please, let's normalize and just have one "statuses" enum.

Copy link
Member Author

Choose a reason for hiding this comment

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

As of now we don't support all statuses on ORG and ENV so I think the distinction makes sense and explaining the code.

use Env_Integration_Status;

/**
* Abstract base class for all integration implementations.
Expand Down Expand Up @@ -48,15 +49,22 @@ abstract class Integration {
protected bool $is_active = false;

/**
* Instance of VipIntegrationConfig. It's useful to have full configuration info
* Array containing all configuration data. It's useful to have full configuration info
* available inside each integration, we can use it for cases like multisite,
* tracking etc.
*
* Note: We don't use this property for activation of the integration.
*
* @var IntegrationVipConfig
* @var array
*/
private IntegrationVipConfig $vip_config;
private array $vip_configs = [];

/**
* A boolean indicating if the integration have multiple configs.
*
* @var bool
*/
protected bool $have_multiple_configs = false;
mehmoodak marked this conversation as resolved.
Show resolved Hide resolved

/**
* Constructor.
Expand All @@ -66,7 +74,7 @@ abstract class Integration {
public function __construct( string $slug ) {
$this->slug = $slug;

add_action( 'switch_blog', array( $this, 'switch_blog_callback' ), 10, 2 );
add_action( 'switch_blog', array( $this, 'switch_blog_callback' ), 10 );
}

/**
Expand Down Expand Up @@ -97,11 +105,13 @@ public function activate( array $options = [] ): void {

/**
* Callback for `switch_blog` filter.
*
* @private
*/
public function switch_blog_callback(): void {
// Updating config to make sure `get_config()` returns config of current blog instead of main site.
if ( isset( $this->vip_config ) ) {
$this->options['config'] = $this->vip_config->get_site_config();
if ( isset( $this->vip_configs ) ) {
$this->options['config'] = $this->get_site_configs( $this->slug );
}
}

Expand All @@ -114,6 +124,17 @@ public function is_active(): bool {
return $this->is_active;
}

/**
* Returns `true` if the integration is enabled in VIP configs else `false`.
*
* @return bool
*
* @private
*/
public function is_active_via_vip(): bool {
return in_array( Env_Integration_Status::ENABLED, $this->get_site_statuses() );
Copy link
Member Author

Choose a reason for hiding this comment

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

For multiple setups enable the integration if any of the source is ENABLED.

Copy link
Contributor

Choose a reason for hiding this comment

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

I still think we absolutely need the concept of a parent integration present here. The parent should be the one deciding if the whole thing should be enabled (and at what version) for the env/subsite.

Copy link
Member Author

Choose a reason for hiding this comment

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

In #5644 we are checking enablement on parent before getting any status from child (src). This PR is mainly just a refactoring PR that supports composable cases.

}

/**
* Return the configuration for this integration.
*
Expand All @@ -135,18 +156,116 @@ public function get_slug(): string {
}

/**
* Set vip_config property.
* Set `vip_configs` property.
*
* @param IntegrationVipConfig $vip_config Instance of IntegrationVipConfig.
* @param array $vip_configs Configurations provided by VIP.
*
* @return void
*
* @private
*/
public function set_vip_configs( array $vip_configs ): void {
$this->vip_configs = $vip_configs;
Copy link
Contributor

Choose a reason for hiding this comment

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

How come we're removing the active check?

Copy link
Member Author

Choose a reason for hiding this comment

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

We are only using set_vip_configs in one place which is already handling the is_active case, therefore removed it.

}

/**
* Get statuses of the integration in context of current site.
*
* @return array<Env_Integration_Status>
*/
private function get_site_statuses() {
Copy link
Member Author

Choose a reason for hiding this comment

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

Method moved from IntegrationVipConfig file and added multi status support.

/**
* Statuses of the integration.
*
* @var array<string>
*/
$statuses = [];

foreach ( $this->vip_configs as $vip_config ) {
if ( $this->get_value_from_config( $vip_config, 'org', 'status' ) === Org_Integration_Status::BLOCKED ) {
return [ Org_Integration_Status::BLOCKED ];
}
Comment on lines +185 to +187
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we instead make these decisions in goop before setting the configmap?

Anything that doesn't belong on a site config (based on inheritance from the client level) shouldn't end up in the configmap in the first place.

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. Currently we can get the info about why a particular integration doesn't have any data in ConfigMap by just looking at the file. If we move it to GOOP then we loses this visibility which effect debugging if user doesn't have enough permissions.

  2. In future we can have integrations that want to show some notification in WP Dashboard if the plugin is blocked on client level. So it's useful to have this info available.

If we decide on making any change then I will make these changes in the next PR because this PR is just doing refactoring.


// Look into network_sites config before and then fallback to env config.
$statuses[] = $this->get_value_from_config( $vip_config, 'network_sites', 'status' ) ??
$this->get_value_from_config( $vip_config, 'env', 'status' );
}

return $statuses;
}

/**
* Get configs of the integration in context of current site.
*
* @return array<array<mixed>> Returns an array of configs if the integration have multiple configs else single config object.
*
* @private
*/
public function set_vip_config( IntegrationVipConfig $vip_config ): void {
if ( ! $this->is_active() ) {
trigger_error( sprintf( 'Configuration info can only assigned if integration is active.' ), E_USER_WARNING ); // phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_trigger_error
public function get_site_configs() {
Copy link
Member Author

Choose a reason for hiding this comment

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

Method moved from IntegrationVipConfig file and added multi configs support.

/**
* Array containing configs of the integration.
*
* @var array<array<mixed>>
*/
$configs = [];

// Get configs of the integration from configurations provided by VIP.
foreach ( $this->vip_configs as $vip_config ) {
if ( is_multisite() ) {
$config = $this->get_value_from_config( $vip_config, 'network_sites', 'config' );

// If network site config is not found then fallback to env config if it exists.
if ( empty( $config ) && true === $this->get_value_from_config( $vip_config, 'env', 'cascade_config' ) ) {
$config = $this->get_value_from_config( $vip_config, 'env', 'config' );
}
} else {
$config = $this->get_value_from_config( $vip_config, 'env', 'config' );
}

if ( ! isset( $config ) ) {
continue;
}

$configs[] = $config;
}

// Return config object if integration have only one config else return all configs.
return ( ! $this->have_multiple_configs && isset( $configs[0] ) ) ? $configs[0] : $configs;
}

/**
* Get config value based on given type and key.
*
* @param array $vip_config Configurations provided by VIP.
* @param string $config_type Type of the config whose data is needed i.e. org, env, network-sites etc.
* @param string $key Key of the config from which we have to extract the data.
*
* @return null|string|array Returns `null` if key is not found, `string` if key is "status" and `array` if key is "config".
*/
private function get_value_from_config( array $vip_config, string $config_type, string $key ) {
$value = null;

if ( ! in_array( $config_type, [ 'org', 'env', 'network_sites' ], true ) ) {
trigger_error( 'config_type param (' . esc_html( $config_type ) . ') must be one of org, env or network_sites.', E_USER_WARNING ); // phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_trigger_error
} elseif ( isset( $vip_config[ $config_type ] ) ) {

// Look for key inside org or env config.
if ( 'network_sites' !== $config_type && isset( $vip_config[ $config_type ][ $key ] ) ) {
return $vip_config[ $config_type ][ $key ];
}
Comment on lines +250 to +253
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not even sure if we need to concept of org configs down at this level either.

When generating configmaps, we're already dealing with only one specific environment. So we can decide what the configs are for that particular environment (based on higher levels if needed), and pass down the resulting env configs along with network site configs if it's a MS.

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently from code point of view we do support having config on org level but in actual we don't have any config from ConfigMap so feel free to let me know if you like me to add a condition here which throws the error if config is provided on org level. Personally I think it is fine as it is.


// Look for key inside network-sites config.
$network_site_id = get_current_blog_id();
if (
'network_sites' === $config_type &&
isset( $vip_config[ $config_type ][ $network_site_id ] ) &&
isset( $vip_config[ $config_type ][ $network_site_id ][ $key ] )
) {
return $vip_config[ $config_type ][ $network_site_id ][ $key ];
}
}

$this->vip_config = $vip_config;
return $value;
}

/**
Expand Down
Loading
Loading