-
Notifications
You must be signed in to change notification settings - Fork 4
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
Split Superset CI build #2994
base: main
Are you sure you want to change the base?
Split Superset CI build #2994
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @shaidar, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request splits the Superset CI pipeline into its own build, allowing for independent iteration on configurations, specifically for authz between Keycloak and Superset, without disrupting the QA/Prod environments. The changes involve refactoring the pipeline definition to support separate CI and main pipelines, introducing configuration options for different environments, and updating the pipeline generation logic.
Highlights
- Pipeline Splitting: The primary goal is to split the Superset CI pipeline from the main pipeline to enable independent development and testing.
- Configuration Management: Introduction of a
PIPELINE_CONFIG
dictionary to manage environment-specific configurations such as branch, image name, Packer build, environment, and stacks. - Resource Creation: Creation of standardized resource creation functions for Git repositories and base resources, enhancing code reusability and maintainability.
- Pipeline Generation: Refactoring the pipeline generation logic to support building different pipeline types (main, ci) based on the specified configuration.
Changelog
Click here to see the changelog
- src/ol_concourse/pipelines/infrastructure/superset/docker_packer_pulumi_pipeline.py
- Added imports for
pathlib
andtyping
modules to support file path manipulation and type hinting (lines 2-3). - Introduced
PipelineConfigDict
TypedDict to define the structure of pipeline configurations (lines 24-29). - Defined
PIPELINE_CONFIG
dictionary to store configurations for 'main' and 'ci' pipelines, including branch, image name, packer build, environment, and stacks (lines 32-46). - Added
ResourceConfigError
,InvalidBranchTypeError
, andInvalidEnvironmentError
exception classes for handling resource configuration errors (lines 50-61). - Created
create_base_resources
function to create base resources (superset_release, docker_code_repo, superset_image) with branch-specific configurations (lines 64-87). - Modified
docker_code_repo
resource to use the configured branch (lines 94-98). - Added logic to determine the image suffix based on the environment (lines 101-103).
- Modified
superset_image
resource to use the environment-specific image name (lines 104-106). - Created
create_pipeline_fragment
function to combine pipeline fragments (lines 114-125). - Created
create_build_job
function to create a standardized build job (lines 128-139). - Created
create_git_resource
function to create standardized git repo resources (lines 165-172). - Added
PipelineError
andInvalidPipelineTypeError
exception classes for handling pipeline errors (lines 175-184). - Refactored
build_superset_docker_pipeline
tobuild_superset_pipeline
and modified it to build pipelines based on the specified pipeline type ('main' or 'ci') (lines 187-230). - Modified the
packer_jobs
fragment to use the configured packer build (lines 232-246). - Modified the
pulumi_jobs_chain
fragment to use the configured stacks (lines 248-253). - Replaced direct resource and job concatenation with
create_pipeline_fragment
(lines 262-273). - Added
PipelineOutputConfig
TypedDict to define the structure of pipeline output configurations (lines 282-284). - Modified the main block to generate both 'main' and 'ci' pipeline definitions and output the corresponding fly commands (lines 287-310).
- Added imports for
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Did you know?
The YAML file format, often used for pipeline definitions, gets its name recursively: YAML Ain't Markup Language.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
The changes in this pull request aim to split the Superset CI pipeline, which is a valuable improvement for development workflow. The introduction of pipeline configurations and resource creation functions enhances modularity and readability. I've provided specific feedback to further improve the code's clarity, error handling, and consistency.
Summary of Findings
- Error Handling: The code introduces custom exception classes, which is good for clarity. However, consider providing more context in the exception messages to aid in debugging.
- Resource Creation: The
create_git_resource
function standardizes git resource creation, but the identifier naming could be more descriptive. - Pipeline Configuration: The
PIPELINE_CONFIG
dictionary provides a clear structure for different pipeline types. Ensure that all necessary configurations are included and validated.
Assessment
The pull request splits the Superset CI pipeline, which is a good step towards isolating changes and preventing disruptions. The code introduces new types, configurations, and functions to manage different pipeline types (main and CI). I've provided some feedback to improve code clarity, error handling, and consistency. Please address these comments and have others review and approve this code before merging.
I think you'll also need to add logic in the PyInfra code and/or docker compose file for the AMI build to be able to use the CI image in the runtime. Otherwise that looks good. |
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request splits the Superset CI pipeline into its own build, allowing for independent iteration on authz configuration between Keycloak and Superset without disrupting QA/Prod. The changes involve modifying the deployment script and the Concourse pipeline definition to handle different configurations for CI, QA, and production environments. The code introduces new functions for determining the environment type and creating resources based on the environment. Overall, the changes seem well-structured and address the intended goal.
Summary of Findings
Merge Readiness
The changes in this pull request appear to be well-structured and address the goal of splitting the Superset CI pipeline. Given the nature of the changes, I recommend that another reviewer with domain expertise in CI/CD pipelines and Superset configurations review the changes before merging. I am unable to directly approve the pull request, and users should have others review and approve this code before merging.
What are the relevant tickets?
#2933 (comment)
Description (What does it do?)
This should split the superset CI pipeline into its own build so that we can iterate on the working to configure authz between Keycloak and Superset without disrupting QA/Prod.