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

mehmoodak · 2024-06-13T13:13:23Z

Description

Refactored "Integrations Framework" to make reading of config files generic i.e. read config by integration type available inside data instead of slug available in file name.
Updated `Integrations Framework" to add the ability of configuring multiple instances of same integration.

Changelog Description

Improved VIP Integrations

Improves reading of config files to make the implementation more generic.
Adds the ability of configuring multiple instances of same integration.

Pre-review checklist

This change works and has been tested locally or in Codespaces.
This change works and has been tested on a sandbox.
This change has relevant unit tests (if applicable).
[N/A] This change uses a rollout method to ease with deployment (if applicable - especially for large scale actions that require writes).
This change has relevant documentation additions / updates (if applicable).
I've created a changelog description that aligns with the provided examples.

Pre-deploy checklist

Update ConfigMaps generating from backend
Reload ConfigMaps of all envs to make sure that config files have type attribute. We do have this reload ability available in backend API.
VIP staff: Ensure any alerts added/updated conform to internal standards (see internal documentation).

Steps to Test

Checkout this PR
Run the vip site locally using vip dev-env start --slug [YOUR_SLUG] (if no env is available then create it)

Create integration config files inside php container and place configs (we have to do it manually because as of now vip dev-env doesn't have support to auto create this folder/file while creating new envs)

docker exec -it [CONTAINER_NAME] /bin/sh
mkdir /wp/config/integrations-config
touch /wp/config/integrations-config/parsely-config.php and adds following content
- parsely can be replaced with any other slug of the integration

return array(
  'type' => 'parsely',
  'env' => array(
  	'status' => 'enabled',
  	'config' => array (
  		'site_id' => 'site.com',
  		'api_secret' => 'site_api_secret',
  		'metadata_secret' => 'site_metadata_secret',
  	)
  ),
  'network_sites' => array (
  	'1' => array (
  		'status' => 'enabled',
  		'config' => array (
  			'site_id' => 'site1.com',
  			'api_secret' => 'site_api_secret_1',
  			'metadata_secret' => 'site_metadata_secret_1',
  		)
  	),
  	'2' => array (
  		'status' => 'enabled',
  		'config' => array (
  			'site_id' => 'site2.com',
  			'api_secret' => 'site_api_secret_2',
  			'metadata_secret' => 'site_metadata_secret_2',
  		)
  	),
  )
);

Create config files for multiple integrations.

touch /wp/config/integrations-config/[SLUG]-123-config.php and adds following content
Replace slug with anything

<?php

return array(
  'type' => 'slug',
  'label' => 'Slug 123',
  'env' => array(
  	'status' => 'enabled',
        'config' => array (
  		'key' => 'slug-123',
  	)
  )
);

touch /wp/config/integrations-config/[SLUG]-456-config.php and adds following content

<?php

return array(
  'type' => 'slug',
  'label' => 'Slug 456',
  'env' => array(
  	'status' => 'enabled',
        'config' => array (
  		'key' => 'slug-456',
  	)
  )
);

Restart vip dev-env to apply changes in these config files.
Verify integration enablement with different combinations of vip configs e.g. wp-parsely plugin is activated with the SiteID and secrets which are provided by VIP (credentails support will be available on v3.9 and above).
Apart from enabling the plugin via VIP config, customer can also enables it by placing \Automattic\VIP\Integrations\activate( 'parsely' ); in plugin-loader.php file so better to also test this flow also.

…s-to-read-files-in-generic-way

…ig methods which are moved

…s-to-read-files-in-generic-way

…s_based_on_vip_configs

codecov · 2024-06-14T18:51:55Z

Codecov Report

Attention: Patch coverage is 91.25000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 29.60%. Comparing base (192958b) to head (fe14e06).
Report is 25 commits behind head on develop.

Files	Patch %	Lines
integrations/vip-integrations-config.php	82.35%	6 Missing ⚠️
vip-integrations.php	50.00%	1 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             develop    #5643      +/-   ##
=============================================
+ Coverage      29.53%   29.60%   +0.06%     
- Complexity      4779     4792      +13     
=============================================
  Files            282      282              
  Lines          20597    20608      +11     
=============================================
+ Hits            6083     6100      +17     
+ Misses         14514    14508       -6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…s-to-read-files-in-generic-way

mehmoodak · 2024-06-17T10:00:02Z

integrations/integration.php

+	 *
+	 * @return array<Env_Integration_Status>
+	 */
+	private function get_site_statuses() {


Method moved from IntegrationVipConfig file and added multi status support.

mehmoodak · 2024-06-17T10:00:13Z

integrations/integration.php

-	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() {


Method moved from IntegrationVipConfig file and added multi configs support.

mehmoodak · 2024-06-17T10:00:59Z

integrations/integrations.php

 	 * @return void
 	 */
-	public function activate_platform_integrations() {
+	public function activate_platform_integrations( VipIntegrationsConfig $vip_integrations_config ) {


For easier mocking providing config as a param.

mehmoodak · 2024-06-17T10:10:47Z

tests/integrations/test-integration.php

-
-			return false;
-		}, E_USER_WARNING );
+		$this->original_error_reporting = setup_custom_error_reporting();


Avoided duplication across files and added following methods:

setup_custom_error_reporting

reset_custom_error_reporting

mehmoodak · 2024-06-17T10:12:47Z

tests/integrations/test-integration.php

@@ -147,19 +141,435 @@ public function test__is_active_returns_false_when_integration_is_not_active():
 		$this->assertFalse( $integration->is_active() );
 	}

-	public function test__set_vip_config_function_throws_error_if_integration_is_not_active(): void {
+	public function test__is_active_via_vip_returns_false_if_empty_config_is_provided(): void {


Moved methods from test-integration-config.php class to here.

Updated methods to allow multiple configs.

integrations/integration.php

…s-to-read-files-in-generic-way

sonarcloud · 2024-06-20T17:13:09Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

WPprodigy · 2024-07-01T20:58:24Z

Pausing on this for now. Will revisit when we're adding in this functionality.

mehmoodak · 2024-08-15T07:24:21Z

Had some discussions with Cantina team and we decided to reopen this Pull Request while keeping the existing proposal as is.

…s-to-read-files-in-generic-way

mehmoodak · 2024-08-15T08:17:44Z

integrations/integration-vip-config.php

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

mehmoodak · 2024-08-15T08:18:17Z

integrations/vip-integrations-config.php

Changed the name from integration-vip-config.php

Refactored get_vip_config_from_file to read all files at once and rename the function to read_config_files

Moved is_active_via_vip, get_site_status, get_site_config and get_value_from_config to Integration class because it makes more sense there. Apart from this added support for multiple configs.

mehmoodak · 2024-08-15T08:19:03Z

tests/integrations/fake-multi-config-integration.php

FakeMultiConfigIntegration class for multi config integration.

mehmoodak · 2024-08-15T08:33:30Z

integrations/integration.php

+	 * @private
+	 */
+	public function is_active_via_vip(): bool {
+		return in_array( Env_Integration_Status::ENABLED, $this->get_site_statuses() );


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

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.

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.

mehmoodak · 2024-08-15T09:46:48Z

tests/utils/utils.php

+/**
+ * Setup custom error handler and returns current level of error reporting.
+ */
+function setup_custom_error_reporting(): int {


Moved duplicated code to following methods

setup_custom_error_reporting

reset_custom_error_reporting

mehmoodak · 2024-08-15T10:48:36Z

Made some minor changes and now this PR is good for review.

rebeccahum · 2024-08-21T17:24:56Z

integrations/integration.php

@@ -7,7 +7,8 @@

 namespace Automattic\VIP\Integrations;

-use Automattic\VIP\Integrations\IntegrationVipConfig;
+use Org_Integration_Status;


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

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

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

rebeccahum · 2024-08-21T18:21:38Z

integrations/integration.php

-		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 set_vip_configs( array $vip_configs ): void {
+		$this->vip_configs = $vip_configs;


How come we're removing the active check?

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

rebeccahum · 2024-08-21T18:47:57Z

integrations/integration.php

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


Besides Composable, how many other integrations will have multiple set-ups? Just wondering, but is this something that can be handled elsewhere? :/

There are some nice benefits to having individual sources be able to cascade down with their own configs like the other integrations. Helps prevent duplication for network sites for example, and keeps things logically operating the same way.

WPprodigy · 2024-08-21T21:57:54Z

integrations/integration.php

@@ -7,7 +7,8 @@

 namespace Automattic\VIP\Integrations;

-use Automattic\VIP\Integrations\IntegrationVipConfig;
+use Org_Integration_Status;


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

WPprodigy · 2024-08-21T22:00:38Z

integrations/integration.php

+			if ( $this->get_value_from_config( $vip_config, 'org', 'status' ) === Org_Integration_Status::BLOCKED ) {
+				return [ Org_Integration_Status::BLOCKED ];
+			}


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.

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.

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.

WPprodigy · 2024-08-21T22:09:14Z

integrations/vip-integrations-config.php

+	 *   'parsely' => array(
+	 *     array(
+	 *       'org'        => array( 'status' => 'blocked' ),
+	 *       'env'        => array(
+	 *         'status' => 'enabled',
+	 *         'config'  => array(),
+	 *       ),
+	 *       'network_sites' => array (
+	 *         1 => array (
+	 *           'status' => 'disabled',
+	 *           'config'  => array(),
+	 *         ),
+	 *         2 => array (
+	 *           'status' => 'enabled',
+	 *           'config'  => array(),
+	 *         ),


I know it's just an example, but I think this is surfacing in the code as a result so wanted to point out here too.

Nothing blocked at the org level should ever make it to the WP site. There's no scenario where we will block on org but enable on env. If blocked at org level or site level, then that should mean the configmap will never see an entry for that integration. It doesn't exist at all.

Thanks for pointing this out. Fixed the example in fe14e06

As per current implementation if the integration is blocked on org level then we don't include any data from site level integration even if there is data.

// Example <?php return array( 'parsely' => array( array( 'org' => array( 'status' => 'blocked' ), ), ), );

WPprodigy · 2024-08-21T22:19:08Z

integrations/integration.php

+			// 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 ];
+			}


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.

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.

WPprodigy · 2024-08-21T22:20:31Z

integrations/integration.php

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


There are some nice benefits to having individual sources be able to cascade down with their own configs like the other integrations. Helps prevent duplication for network sites for example, and keeps things logically operating the same way.

WPprodigy · 2024-08-21T22:22:04Z

integrations/integration.php

+	 * @private
+	 */
+	public function is_active_via_vip(): bool {
+		return in_array( Env_Integration_Status::ENABLED, $this->get_site_statuses() );


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.

sonarcloud · 2024-08-22T08:34:58Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

mehmoodak added 2 commits June 12, 2024 15:42

🔨 REFACTOR: reading of VIP Integrations config

f415cae

ADDS: Support for handling multiple configs of the integration

c89c999

mehmoodak self-assigned this Jun 13, 2024

mehmoodak added 2 commits June 13, 2024 18:16

Merge branch 'develop' into cafe-919/update-IntegrationVipConfig-clas…

c962f96

…s-to-read-files-in-generic-way

🔨 REFACTOR: reduce return statements

107649d

mehmoodak changed the title ~~Improves reading of config files in IntegrationVIPConfig class and add support for handling multiple configs~~ Integrations: Improves reading of config files and add support for multiple configs Jun 13, 2024

mehmoodak added 6 commits June 13, 2024 18:31

Merge branch 'develop' into cafe-919/update-IntegrationVipConfig-clas…

3d654bb

…s-to-read-files-in-generic-way

🔨 REFACTOR: incorporate suggestion of merging if statements

8b2916a

➕ ADDS: tests for VIP_Integrations_Config_Test

9d0ee40

💫 UPDATE: VIP_Integrations_Test

a47bee0

💫 UPDATE: VIP_Integration_Test

d77380b

💫 UPDATE: VIP_Integration_Test to include tests of IntegrationVipConf…

13fdecc

…ig methods which are moved

mehmoodak force-pushed the cafe-919/update-IntegrationVipConfig-class-to-read-files-in-generic-way branch from dcd3c2d to 13fdecc Compare June 14, 2024 18:28

mehmoodak added 4 commits June 14, 2024 23:30

➖ REMOVE: removed unused field

0da775d

Merge branch 'develop' into cafe-919/update-IntegrationVipConfig-clas…

accfa94

…s-to-read-files-in-generic-way

👌 IMPROVE: test name

de0b802

🐛 FIX: test activate_platform_integrations_are_activating_integration…

a83322d

…s_based_on_vip_configs

mehmoodak force-pushed the cafe-919/update-IntegrationVipConfig-class-to-read-files-in-generic-way branch 3 times, most recently from 4377568 to c5586c6 Compare June 16, 2024 19:01

👌 IMPROVE: VIP_Integrations_Config_Test

4653616

mehmoodak force-pushed the cafe-919/update-IntegrationVipConfig-class-to-read-files-in-generic-way branch from c5586c6 to 4653616 Compare June 16, 2024 19:04

mehmoodak added 2 commits June 17, 2024 00:49

➕ ADDS: multi config integrations test

ae48d59

Merge branch 'develop' into cafe-919/update-IntegrationVipConfig-clas…

6133b8e

…s-to-read-files-in-generic-way

mehmoodak changed the title ~~Integrations: Improves reading of config files and add support for multiple configs~~ VIP Integrations: Improves reading of config files and add support for multiple configs of same integration Jun 17, 2024

mehmoodak changed the title ~~VIP Integrations: Improves reading of config files and add support for multiple configs of same integration~~ VIP Integrations: Improves reading of config files and add multiple configs support of same integration Jun 17, 2024

mehmoodak requested review from maxschmeling and chriszarate June 17, 2024 10:14

mehmoodak commented Jun 17, 2024

View reviewed changes

mehmoodak marked this pull request as ready for review June 17, 2024 10:15

mehmoodak requested a review from a team as a code owner June 17, 2024 10:15

rebeccahum reviewed Jun 18, 2024

View reviewed changes

integrations/integration.php Outdated Show resolved Hide resolved

mehmoodak added 3 commits June 20, 2024 22:11

👌 IMPROVE: comments

f575c06

👌 IMPROVE: variable name

251976d

Merge branch 'develop' into cafe-919/update-IntegrationVipConfig-clas…

5ba7e30

…s-to-read-files-in-generic-way

WPprodigy closed this Jul 1, 2024

mehmoodak reopened this Aug 15, 2024

mehmoodak added 3 commits August 15, 2024 13:07

Merge branch 'develop' into cafe-919/update-IntegrationVipConfig-clas…

da24f72

…s-to-read-files-in-generic-way

👌 IMPROVE: PHPDocs

d36f954

🐛 FIX: sonar cloud issue

45da1ae

mehmoodak commented Aug 15, 2024

View reviewed changes

mehmoodak requested review from WPprodigy and rebeccahum August 15, 2024 10:08

rebeccahum reviewed Aug 21, 2024

View reviewed changes

WPprodigy reviewed Aug 21, 2024

View reviewed changes

🐛 FIX: ConfigMap example

fe14e06

mehmoodak requested review from WPprodigy and rebeccahum August 22, 2024 08:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mehmoodak commented Jun 13, 2024 •

edited

Loading

codecov bot commented Jun 14, 2024 •

edited

Loading

mehmoodak Jun 17, 2024

mehmoodak Jun 17, 2024

mehmoodak Jun 17, 2024

mehmoodak Jun 17, 2024

mehmoodak Jun 17, 2024

sonarcloud bot commented Jun 20, 2024

WPprodigy commented Jul 1, 2024

mehmoodak commented Aug 15, 2024

mehmoodak Aug 15, 2024

mehmoodak Aug 15, 2024

mehmoodak Aug 15, 2024

mehmoodak Aug 15, 2024

WPprodigy Aug 21, 2024

mehmoodak Aug 22, 2024

mehmoodak Aug 15, 2024

mehmoodak commented Aug 15, 2024

rebeccahum Aug 21, 2024

WPprodigy Aug 21, 2024

mehmoodak Aug 22, 2024

rebeccahum Aug 21, 2024

mehmoodak Aug 22, 2024

rebeccahum Aug 21, 2024

WPprodigy Aug 21, 2024

WPprodigy Aug 21, 2024

WPprodigy Aug 21, 2024

mehmoodak Aug 22, 2024

WPprodigy Aug 21, 2024

mehmoodak Aug 22, 2024

WPprodigy Aug 21, 2024

mehmoodak Aug 22, 2024

WPprodigy Aug 21, 2024

WPprodigy Aug 21, 2024

sonarcloud bot commented Aug 22, 2024

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

Are you sure you want to change the base?

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

Conversation

mehmoodak commented Jun 13, 2024 • edited Loading

Description

Changelog Description

Improved VIP Integrations

Pre-review checklist

Pre-deploy checklist

Steps to Test

codecov bot commented Jun 14, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Jun 20, 2024

Quality Gate passed

WPprodigy commented Jul 1, 2024

mehmoodak commented Aug 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mehmoodak commented Aug 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Aug 22, 2024

Quality Gate passed

mehmoodak commented Jun 13, 2024 •

edited

Loading

codecov bot commented Jun 14, 2024 •

edited

Loading