feat(refactor): introduce new refactored architecture and environment setup #3531
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a comprehensive refactored architecture for the Red Hat Developer Hub (RHDH) CI/CD system, transitioning from a monolithic script approach to a modular, maintainable structure. The refactor significantly improves code organization, testing capabilities, and deployment flexibility.
- Modular architecture with separate modules for platform detection, deployment strategies, and testing
- Enhanced environment configuration system with local testing support and validation
- Comprehensive Kubernetes resource management for various deployment scenarios
Reviewed Changes
Copilot reviewed 94 out of 96 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| .ibm/refactored/openshift-ci-tests.sh | Main entry point with job routing and execution framework |
| .ibm/refactored/modules/ | Core modules for logging, k8s operations, deployment strategies, and platform detection |
| .ibm/refactored/value_files/ | Helm values configurations for different deployment scenarios (showcase, RBAC, cloud providers) |
| .ibm/refactored/resources/ | Kubernetes resource definitions for services, operators, and testing infrastructure |
Comments suppressed due to low confidence (1)
.ibm/refactored/modules/config-validation.sh:1
- The base64 regex pattern is too permissive and could match regular strings. Consider adding a minimum length check or more specific validation to avoid false positives with short strings that happen to match the base64 character set.
#!/usr/bin/env bash
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| - apiVersion: "v1beta1" | ||
| group: "tekton.dev" | ||
| plural: "pipelines" | ||
| - apiVersion: v1beta1 | ||
| group: tekton.dev | ||
| plural: pipelineruns | ||
| - apiVersion: v1beta1 |
There was a problem hiding this comment.
Using deprecated v1beta1 API version for Tekton resources. Consider upgrading to v1 API version for better long-term compatibility, as v1beta1 may be removed in future Tekton versions.
| - apiVersion: "v1beta1" | |
| group: "tekton.dev" | |
| plural: "pipelines" | |
| - apiVersion: v1beta1 | |
| group: tekton.dev | |
| plural: pipelineruns | |
| - apiVersion: v1beta1 | |
| - apiVersion: "v1" | |
| group: "tekton.dev" | |
| plural: "pipelines" | |
| - apiVersion: v1 | |
| group: tekton.dev | |
| plural: pipelineruns | |
| - apiVersion: v1 |
Copilot uses AI. Check for mistakes.
| - apiVersion: "v1beta1" | ||
| group: "tekton.dev" | ||
| plural: "pipelines" | ||
| - apiVersion: v1beta1 | ||
| group: tekton.dev | ||
| plural: pipelineruns | ||
| - apiVersion: v1beta1 |
There was a problem hiding this comment.
Using deprecated v1beta1 API version for Tekton resources. Consider upgrading to v1 API version for better long-term compatibility, as v1beta1 may be removed in future Tekton versions.
| - apiVersion: "v1beta1" | |
| group: "tekton.dev" | |
| plural: "pipelines" | |
| - apiVersion: v1beta1 | |
| group: tekton.dev | |
| plural: pipelineruns | |
| - apiVersion: v1beta1 | |
| - apiVersion: "v1" | |
| group: "tekton.dev" | |
| plural: "pipelines" | |
| - apiVersion: v1 | |
| group: tekton.dev | |
| plural: pipelineruns | |
| - apiVersion: v1 |
Copilot uses AI. Check for mistakes.
| - apiVersion: "v1beta1" | ||
| group: "tekton.dev" | ||
| plural: "pipelines" | ||
| - apiVersion: v1beta1 | ||
| group: tekton.dev | ||
| plural: pipelineruns | ||
| - apiVersion: v1beta1 |
There was a problem hiding this comment.
Using deprecated v1beta1 API version for Tekton resources. Consider upgrading to v1 API version for better long-term compatibility, as v1beta1 may be removed in future Tekton versions.
| - apiVersion: "v1beta1" | |
| group: "tekton.dev" | |
| plural: "pipelines" | |
| - apiVersion: v1beta1 | |
| group: tekton.dev | |
| plural: pipelineruns | |
| - apiVersion: v1beta1 | |
| - apiVersion: "v1" | |
| group: "tekton.dev" | |
| plural: "pipelines" | |
| - apiVersion: v1 | |
| group: tekton.dev | |
| plural: pipelineruns | |
| - apiVersion: v1 |
Copilot uses AI. Check for mistakes.
| - apiVersion: "v1beta1" | ||
| group: "tekton.dev" | ||
| plural: "pipelines" | ||
| - apiVersion: v1beta1 | ||
| group: tekton.dev | ||
| plural: pipelineruns | ||
| - apiVersion: v1beta1 |
There was a problem hiding this comment.
Using deprecated v1beta1 API version for Tekton resources. Consider upgrading to v1 API version for better long-term compatibility, as v1beta1 may be removed in future Tekton versions.
| - apiVersion: "v1beta1" | |
| group: "tekton.dev" | |
| plural: "pipelines" | |
| - apiVersion: v1beta1 | |
| group: tekton.dev | |
| plural: pipelineruns | |
| - apiVersion: v1beta1 | |
| - apiVersion: "v1" | |
| group: "tekton.dev" | |
| plural: "pipelines" | |
| - apiVersion: v1 | |
| group: tekton.dev | |
| plural: pipelineruns | |
| - apiVersion: v1 |
Copilot uses AI. Check for mistakes.
| - apiVersion: "v1beta1" | ||
| group: "tekton.dev" | ||
| plural: "pipelines" | ||
| - apiVersion: v1beta1 | ||
| group: tekton.dev | ||
| plural: pipelineruns | ||
| - apiVersion: v1beta1 |
There was a problem hiding this comment.
Using deprecated v1beta1 API version for Tekton resources. Consider upgrading to v1 API version for better long-term compatibility, as v1beta1 may be removed in future Tekton versions.
| - apiVersion: "v1beta1" | |
| group: "tekton.dev" | |
| plural: "pipelines" | |
| - apiVersion: v1beta1 | |
| group: tekton.dev | |
| plural: pipelineruns | |
| - apiVersion: v1beta1 | |
| - apiVersion: "v1" | |
| group: "tekton.dev" | |
| plural: "pipelines" | |
| - apiVersion: v1 | |
| group: tekton.dev | |
| plural: pipelineruns | |
| - apiVersion: v1 |
Copilot uses AI. Check for mistakes.
| local namespace="$1" | ||
| local junit_file="$2" | ||
|
|
||
| [[ "${OPENSHIFT_CI}" != "true" ]] && return 0 | ||
|
|
||
| local artifacts_url=$(get_artifacts_url "${namespace}") | ||
|
|
||
| # Replace attachments with links to OpenShift CI storage | ||
| sed -i.bak "s#\[\[ATTACHMENT|\(.*\)\]\]#${artifacts_url}/\1#g" "${junit_file}" | ||
|
|
||
| # Fix XML property tags format for Data Router compatibility | ||
| # Convert to self-closing format | ||
| sed -i.bak 's#</property>##g' "${junit_file}" | ||
| sed -i.bak 's#<property name="\([^"]*\)" value="\([^"]*\)">#<property name="\1" value="\2"/>#g' "${junit_file}" | ||
|
|
||
| # Copy to shared directory for CI | ||
| cp "${junit_file}" "${SHARED_DIR}/junit-results-${namespace}.xml" 2>/dev/null || true | ||
|
|
||
| log_info "JUnit results adapted for Data Router and saved" | ||
| } |
There was a problem hiding this comment.
Multiple sed operations modify the same file sequentially, creating .bak files that aren't cleaned up. Consider combining operations or explicitly cleaning up backup files to avoid accumulation of temporary files.
Copilot uses AI. Check for mistakes.
|
The image is available at: |
|
/review |
|
/describe |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR TypeEnhancement Description• Major architectural refactor: Complete restructuring of CI/CD scripts into modular, reusable components with improved maintainability
|
| Relevant files | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Enhancement | 29 files
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Configuration changes | 4 files
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Tests | 3 files
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Additional files | 57 files
|
There was a problem hiding this comment.
Hi @gustavolira
I like many ideas in this PR overall. But making so many changes in one PR is making it impossible to review properly and discuss all the changes.
- I suggest we start by setting up an OpenShift CI nightly job(s) and optional PR check(s), that would use the
refactoringfolder as an entry point. This will give everyone access to logs and allow us to verify how this script behaves in CI. - Then you can work on the base refactoring. Start with OCP jobs, maybe just Helm chart first, then add the operator. Review this PR and merge it.
- When this is working, we can reporting, gathering artifacts etc.
- Only after that, I'd add other cloud providers like AKS, GKE, EKS, and special tests, like
auth-providersandupgradetests. And again, I'd first verify that the scripts work well with all of them before we merge it. It can also be one PR for each provider, if we need it.
On the architectural level:
- Do we need to keep the
openshift-ci-tests.shand all the decision making based onJOB_NAME? We can instead configure each job use different starting point. This is the point for OCP Helm PR checks e.g.: https://github.com/openshift/release/blob/6a5999d35c9bedca66a608cf5a9a2ad6bff49712/ci-operator/step-registry/redhat-developer/rhdh/ocp/helm/redhat-developer-rhdh-ocp-helm-commands.sh#L147 - Even Makefile relies on
openshift-ci-tests.shandJOB_NAMEand I don't think it's needed and I see it as a weak point. - There are many ShellCheck errors and warning in the code and the sourcing is not working with ShellCheck and BashIDE. I think it's a great tool and using it can help you and Cursor to pick up the errors and warning earlier and fix it during the code generation.
- I'd keep the linting and formatting on
yarnand configuration inpackage.jsonand avoid duplicating it in the Makefile.
Thanks a lot for the detailed feedback! Another reason is that most of this refactor was generated and structured with AI assistance, which means the code was produced as a complete, consistent set. I agree that the auth-providers part can be postponed; that one is easier to migrate later. My proposal would be to keep the refactored/ folder temporarily alongside the current code, and create new OpenShift CI jobs pointing to the refactored scripts. Once we’re confident that everything runs smoothly in CI, we can then fully switch over, deleting the old scripts and keeping only the refactored version. This approach should give us a safer and more gradual migration while keeping the current pipeline stable. |
about the other points like openshift-ci-tests.sh and Shellcheck I also agree with you |
… setup - Added new refactored scripts and modules for improved CI/CD processes. - Introduced environment configuration files and example scripts for local testing. - Created documentation for the refactored architecture and usage of cursor rules. - Updated .gitignore to include new environment override files. - Added various Kubernetes resource configurations for deployment and service accounts. This refactor enhances maintainability and simplifies the deployment process.
…provider support - Introduced a new ShellCheck configuration for improved script linting. - Updated the major chart version from 1.7 to 1.8 across various scripts and configurations. - Added upgrade testing functionality to validate upgrades from previous releases. - Implemented new entry points for auth providers, cleanup, and deployment jobs. - Enhanced documentation to include upgrade flow and cloud provider deployment details. - Refactored Makefile to streamline CI/CD targets and improve usability. This update significantly improves the deployment process and testing capabilities for RHDH.
gustavolira
force-pushed
the
main-refactor-sh
branch
from
02af037 to
fd51b04
Compare
|
@zdrapela make another improvement with your suggestion. I think a good next step would be create some jobs on Openshift CI enabling run the handlers from the refactored folder and see the results. |
|
The image is available at: |
There are 11 jobs in the scope, if I'm counting correctly. How many of them have you tested to see if they work correctly? To do an AI refactor is one thing, and to ensure the code for all the jobs is doing exactly what it's supposed to is another thing. If we do the refactor properly, it should be low effort to split it into pieces. I wouldn't merge the code into the repository if we don't know if it works. The proper way is to split it into pieces, review them, see if they work, and then merge them. And most importantly, this PR is adding 20,000 lines. If any PR is impossible to review, it's this one. |
I agree with you. To start testing, I was thinking of creating all those new jobs in OpenShift CI so that we can test each one individually. |
2 participants
includes AI-generated content
This refactor enhances maintainability and simplifies the deployment process.
Which issue(s) does this PR fix
PR acceptance criteria
Please make sure that the following steps are complete:
How to test changes / Special notes to the reviewer