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

Refactor CDR and CIS workflows #2491

Open
wants to merge 50 commits into
base: main
Choose a base branch
from
Open

Conversation

gurevichdmitry
Copy link
Collaborator

@gurevichdmitry gurevichdmitry commented Sep 10, 2024

Summary of your changes

This PR introduces the capability to install the CDR infrastructure with or without the CIS infrastructure. This enhancement allows for more flexibility in setting up the infrastructure based on the specific needs of the deployment.

Key Changes:

  • Added support for installing CDR independently or in combination with CIS.
  • Adjusted configuration options to ensure seamless integration between CDR and CIS when both are enabled.

Screenshot/Data

Screenshot 2024-11-14 at 11 11 07

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • I have added the necessary README/documentation (if appropriate)

Successful Workflow Test Runs

Copy link

mergify bot commented Sep 10, 2024

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b dg-refactor-cloud-logs-wf upstream/dg-refactor-cloud-logs-wf
git merge upstream/main
git push upstream dg-refactor-cloud-logs-wf

Copy link

mergify bot commented Sep 10, 2024

This pull request does not have a backport label. Could you fix it @gurevichdmitry? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v./d./d./d is the label to automatically backport to the 8./d branch. /d is the digit
    NOTE: backport-v8.x has been added to help with the transition to the new branch 8.x.

@gurevichdmitry gurevichdmitry marked this pull request as ready for review September 12, 2024 12:32
@gurevichdmitry gurevichdmitry requested a review from a team as a code owner September 12, 2024 12:32
@gurevichdmitry
Copy link
Collaborator Author

Comment on lines +23 to +27
cis-infra:
required: false
description: "Deploy the CIS infrastructure"
type: boolean
default: false
Copy link
Collaborator

@orouz orouz Sep 16, 2024

Choose a reason for hiding this comment

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

it looks like each workflow - cdr infra or create environment can run either infra or both. personally i think a single workflow with 2 checkboxes would be simpler, but maybe i'm missing smth

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After refactoring the Create Environment workflow, we can provide a choice, such as a combo box with options to select CIS, CDR, or All. From the CDR perspective, we only need to specify whether we want to install CIS as well. In this case, I believe a checkbox would be preferable.

Comment on lines +101 to +105
infra-type:
description: "Type of infrastructure to create"
type: string
required: false
default: false
default: "cis"
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe a boolean for CDR and CIS is better? otherwise the string needs to be known to whoever runs this workflow

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This will be resolved by refactoring the test-environment workflow in this task.

Copy link
Collaborator

@orouz orouz Sep 16, 2024

Choose a reason for hiding this comment

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

consider splitting this workflow to multiple actions, like deploy-elk, deploy-cdr, deploy-cis and using whichever is required based on inputs.

i think doing so will make the workflow more approachable for future updates as it is getting a bit complex. it'll also remove the need of various wrapper script like manage_infrastructure.sh and set_cloud_env_params.sh

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This will be resolved by refactoring the test-environment workflow in this task. Regarding wrapping scripts around the Terraform code, I'm not sure I agree with that approach, as it would centralize all logic related to deployment. However, we can reconsider this when the new actions for CIS and CDR are created.

Copy link

mergify bot commented Oct 10, 2024

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b dg-refactor-cloud-logs-wf upstream/dg-refactor-cloud-logs-wf
git merge upstream/main
git push upstream dg-refactor-cloud-logs-wf

@gurevichdmitry gurevichdmitry linked an issue Nov 13, 2024 that may be closed by this pull request
2 tasks
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.

Improve Fleet API response compatibility for version 9.x
2 participants