Skip to content

Conversation

@JessicaJHee
Copy link
Member

Description

Adds E2E test to the auth-provider tests that confirms that the MY_CUSTOM_ANNOTATION was added to users by the github-org custom transformer. Tests are passing locally.

  • the transformer plugin is published here

Which issue(s) does this PR fix

PR acceptance criteria

Please make sure that the following steps are complete:

  • GitHub Actions are completed and successful
  • Unit Tests are updated and passing
  • E2E Tests are updated and passing
  • Documentation is updated if necessary (requirement for new features)
  • Add a screenshot if the change is UX/UI related

How to test changes / Special notes to the reviewer

Signed-off-by: Jessica He <jhe@redhat.com>
@rhdh-qodo-merge
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

RHIDP-10977 - Partially compliant

Compliant requirements:

  • Install and implement tests in the auth-provider tests to ensure the transformer applies after ingestion.
  • Acceptance criteria: add a custom annotation via the transformer module and ensure the transformation is applied properly.

Non-compliant requirements:

  • Create a sample transformer plugin in the e2e-tests folder.

Requires further human verification:

  • Manually build the OCI artifact for the transformer.
  • Verify the referenced OCI image/tag exists, is accessible in CI, and contains the expected plugin artifact.
  • Confirm the transformer is actually applied post-ingestion in a real E2E run (not just that the helper method works).
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🔒 Security concerns

Supply chain risk:
The PR pulls/enables a dynamic plugin from an external OCI URL (oci://quay.io/...:v0.1.0!...). If the tag is mutable or the registry content changes, CI could execute unintended code. Prefer pinning to an image digest and ensuring provenance/scanning/signing policies for the artifact.

⚡ Recommended focus areas for review

Supply Chain

The test enables a dynamic plugin from an external OCI reference. This can introduce flakiness and supply-chain risk if the image is moved/retagged, becomes unavailable, or changes unexpectedly. Consider pinning to an immutable digest and/or hosting in an officially managed registry/namespace to stabilize CI.

// Use local path for local development, OCI path for CI/CD
const transformerPluginPath = this.isRunningLocal
  ? "./dynamic-plugins/dist/@internal/backstage-plugin-catalog-backend-module-github-org-transformer-dynamic"
  : "oci://quay.io/rh-ee-jhe/catalog-github-org-transformer:v0.1.0!internal-backstage-plugin-catalog-backend-module-github-org-transformer";

this.setDynamicPluginEnabled(transformerPluginPath, true);
Test Robustness

The assertions check for a specific annotation key/value on users; this may be sensitive to timing/ingestion latency. Consider adding retry/backoff (or a “wait until catalog updated” helper) around the annotation checks to reduce intermittent failures in slower CI environments.

expect(
  await deployment.checkUserHasAnnotation(
    "rhdhqeauthadmin",
    "MY_CUSTOM_ANNOTATION",
    "rhdhqeauthadmin",
  ),
).toBe(true);
expect(
  await deployment.checkUserHasAnnotation(
    "rhdhqeauth1",
    "MY_CUSTOM_ANNOTATION",
    "rhdhqeauth1",
  ),
).toBe(true);
📚 Focus areas based on broader codebase context

Configurability

The transformer plugin OCI reference is hardcoded to v0.1.0, which can make CI/CD upgrades and downstream overrides harder (e.g., needing PRs for every tag bump). Consider making the OCI URL/tag configurable via a value/env-driven mechanism (or at least a single central value) so different environments can override it without changing the repo. (Ref 6, Ref 7)

- package: oci://quay.io/rh-ee-jhe/catalog-github-org-transformer:v0.1.0!internal-backstage-plugin-catalog-backend-module-github-org-transformer
  disabled: true

Reference reasoning: The referenced YAML in the codebase demonstrates a pattern of avoiding hardcoded image references by allowing runtime/environment substitution (e.g., comments indicating values will be replaced by related env vars). Applying a similar approach here would keep test/plugin dependencies easier to update and override across environments.

📄 References
  1. redhat-developer/rhdh-chart/charts/backstage/templates/tests/test-secret.yaml [1-3]
  2. redhat-developer/rhdh-chart/charts/backstage/templates/tests/test-connection.yaml [0-2]
  3. redhat-developer/rhdh-chart/charts/orchestrator-infra/templates/tests/infra-test.yaml [0-2]
  4. redhat-developer/rhdh-chart/charts/orchestrator-software-templates-infra/templates/tests/infra-test.yaml [0-2]
  5. redhat-developer/rhdh-chart/charts/orchestrator-infra/values.yaml [37-41]
  6. redhat-developer/rhdh-operator/pkg/model/testdata/rhdh-deployment.yaml [36-58]
  7. redhat-developer/rhdh-operator/pkg/model/testdata/janus-deployment.yaml [36-58]
  8. redhat-developer/rhdh-operator/dist/rhdh/install.yaml [2237-2246]

@rhdh-qodo-merge
Copy link

PR Type

Tests, Enhancement


Description

  • Add E2E test validating custom annotation added by GitHub org transformer

  • Implement checkUserHasAnnotation method to verify user metadata annotations

  • Configure transformer plugin for both local development and CI/CD environments

  • Register custom transformer plugin in dynamic plugin configurations


File Walkthrough

Relevant files
Tests
github.spec.ts
Add custom annotation verification assertions                       

e2e-tests/playwright/e2e/auth-providers/github.spec.ts

  • Added assertions to verify MY_CUSTOM_ANNOTATION is present on test
    users
  • Tests check both "rhdhqeauthadmin" and "rhdhqeauth1" users have
    expected annotations
+15/-0   
Enhancement
rhdh-deployment.ts
Add annotation checking and transformer plugin setup         

e2e-tests/playwright/utils/authentication-providers/rhdh-deployment.ts

  • Implemented new checkUserHasAnnotation method to query and validate
    user annotations via API
  • Added conditional logic to use local or OCI path for transformer
    plugin based on environment
  • Enabled transformer plugin in dynamic plugin configuration
+23/-0   
Configuration changes
values_showcase-auth-providers.yaml
Register transformer plugin in showcase values                     

.ibm/pipelines/value_files/values_showcase-auth-providers.yaml

  • Registered custom GitHub org transformer plugin from OCI registry
  • Added plugin configuration with disabled flag for showcase environment
+2/-0     
dynamic-plugins-config.yaml
Register transformer plugin in E2E config                               

e2e-tests/playwright/utils/authentication-providers/yamls/dynamic-plugins-config.yaml

  • Added custom transformer plugin OCI reference to dynamic plugins
    configuration
  • Configured plugin with disabled flag for E2E test environment
+2/-0     

@sonarqubecloud
Copy link

@JessicaJHee
Copy link
Member Author

/test e2e-ocp-operator-auth-providers-nightly

@rhdh-qodo-merge
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Add transformer to provider config

Register the custom transformer in the githubOrg provider configuration to
ensure it runs during entity ingestion.

e2e-tests/playwright/utils/authentication-providers/rhdh-deployment.ts [1149-1164]

 this.setAppConfigProperty("catalog.providers", {
   githubOrg: [
     {
       // existing GitHub provider config
+      transformers: [
+        "internal-backstage-plugin-catalog-backend-module-github-org-transformer",
+      ],
     },
   ],
 });

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 9

__

Why: This suggestion identifies a critical bug; without registering the transformer in the provider configuration, the new feature would not work, causing the new tests to fail.

High
Correct YAML indentation

Correct the YAML indentation for the new plugin entries in the plugins list to
ensure the file parses correctly.

.ibm/pipelines/value_files/values_showcase-auth-providers.yaml [8-11]

 - package: ./dynamic-plugins/dist/backstage-plugin-catalog-backend-module-github-org-dynamic
-   disabled: true
+  disabled: true
 - package: oci://quay.io/rh-ee-jhe/catalog-github-org-transformer:v0.1.0!internal-backstage-plugin-catalog-backend-module-github-org-transformer
-   disabled: true
+  disabled: true
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies and fixes a YAML indentation error that would cause parsing to fail, which is a critical issue for the pipeline configuration.

Medium
Avoid hardcoding OCI image paths

Replace the hardcoded OCI image path for the transformer plugin with an
environment variable to improve maintainability and flexibility.

e2e-tests/playwright/utils/authentication-providers/rhdh-deployment.ts [1142-1147]

 // Use local path for local development, OCI path for CI/CD
+const transformerPluginOciPath =
+  process.env.CATALOG_GHA_TRANSFORMER_OCI_PATH ||
+  "oci://quay.io/rh-ee-jhe/catalog-github-org-transformer:v0.1.0!internal-backstage-plugin-catalog-backend-module-github-org-transformer";
 const transformerPluginPath = this.isRunningLocal
   ? "./dynamic-plugins/dist/@internal/backstage-plugin-catalog-backend-module-github-org-transformer-dynamic"
-  : "oci://quay.io/rh-ee-jhe/catalog-github-org-transformer:v0.1.0!internal-backstage-plugin-catalog-backend-module-github-org-transformer";
+  : transformerPluginOciPath;
 
 this.setDynamicPluginEnabled(transformerPluginPath, true);
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: This is a strong suggestion that improves the maintainability and flexibility of the test suite by removing a hardcoded OCI image path and replacing it with an environment variable.

Medium
Avoid creating new API helper instances

Refactor checkUserHasAnnotation to avoid creating a new APIHelper instance on
every call by accepting a pre-configured instance as a parameter.

e2e-tests/playwright/utils/authentication-providers/rhdh-deployment.ts [1419-1434]

 async checkUserHasAnnotation(
+  api: APIHelper,
   user: string,
   annotationKey: string,
   expectedValue: string,
 ): Promise<boolean> {
-  const api = new APIHelper();
-  await api.UseStaticToken(this.staticToken);
-  await api.UseBaseUrl(await this.computeBackstageBackendUrl());
   const userEntity: UserEntity = await api.getCatalogUserFromAPI(user);
   const annotations = userEntity.metadata?.annotations || {};
   const actualValue = annotations[annotationKey];
   console.log(
     `Checking user ${user} has annotation ${annotationKey}=${expectedValue}, actual value: ${actualValue}`,
   );
   return actualValue === expectedValue;
 }
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies an inefficiency by creating a new APIHelper instance on each call and proposes a valid refactoring to improve performance and code quality.

Low
  • More

@github-actions
Copy link
Contributor

The image is available at:

/test e2e-ocp-helm

@JessicaJHee
Copy link
Member Author

/test e2e-ocp-operator-auth-providers-nightly

Copy link
Member

@albarbaro albarbaro left a comment

Choose a reason for hiding this comment

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

The values_showcase-auth-providers.yaml file is not used anymore by the auth-providers job, it uses the Operator to deploy RHDH. The changes in the rhdhdeployment.ts file are enough to enable the plugin.
I'll remove the values file soon in a dedicated PR.
Rest looks good to me! Thanks!

@albarbaro
Copy link
Member

/lgtm

@openshift-ci
Copy link

openshift-ci bot commented Jan 15, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: albarbaro

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@albarbaro
Copy link
Member

/test e2e-ocp-helm

1 similar comment
@JessicaJHee
Copy link
Member Author

/test e2e-ocp-helm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants