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

feat!: DBTP-1506 Update platform helper codebase deploy command to trigger manual deployment pipeline #698

Conversation

tony-griffin
Copy link
Contributor

@tony-griffin tony-griffin commented Dec 19, 2024

Addresses DBTP-1506

  • Breaking change that will require a coordinated merge with the codebase pipeline work that's happening. Bumped the default Terraform Platform Modules version to 7 for this reason. Version 7 is to be released. Version 6 will be the work on the IP filter spoofing that's still waiting to be merged. Then this PR and John's work on the codebase pipelines will be in version 7.

  • Required to pull in the terraform code that John is working on for the codebase pipeline and re-deploy my environment

  • That work provisions an IAM role <application>-<environment>-codebase-pipeline-deploy which is required so that the manual codebase pipeline can assume it and run things in the user’s environment.

  • Required to update demodjango-deploy/terraform/environments/<environment>/main.tf with a new parameter, pipeline_account_id, that gets passed to the terraform-platform-modules/extensions module

args = {
    application    = "demodjango"
    services       = local.config["extensions"]
    dns_account_id = local.env_config["tony"]["accounts"]["dns"]["id"]
    pipeline_account_id = local.env_config["*"]["accounts"]["deploy"]["id"]
  }

Checklist:

Title:

  • Scope included as per conventional commits
  • Ticket reference included (unless it's a quick out of ticket thing)

Description:

  • Link to ticket included (unless it's a quick out of ticket thing)
  • Includes tests (or an explanation for why it doesn't)
  • If the work includes user interface changes, before and after screenshots included in description
  • Includes any applicable changes to the documentation in this code base
  • Includes link(s) to any applicable changes to the documentation in the DBT Platform Documentation (can be to a pull request)

Tasks:

  • Run the end to end tests for this branch and confirm that they are passing

@tony-griffin tony-griffin changed the title feat: DBTP-1506 Update platform helper codebase deploy command to trigger manual deployment pipeline feat!: DBTP-1506 Update platform helper codebase deploy command to trigger manual deployment pipeline Dec 20, 2024
@codecov-commenter
Copy link

codecov-commenter commented Dec 20, 2024

❌ 5 Tests Failed:

Tests completed Failed Passed Skipped
665 5 660 3
View the top 3 failed tests by shortest run time
tests/platform_helper/domain/test_codebase.py::test_codebase_deploy_does_not_trigger_pipeline_build_without_confirmation
Stack Traces | 0.005s run time
.../platform_helper/domain/test_codebase.py:339: in test_codebase_deploy_does_not_trigger_pipeline_build_without_confirmation
    codebase.deploy("test-application", "development", "application", "ab1c23d")
.../dbt_platform_helper/domain/codebase.py:164: in deploy
    self.confirm,
E   AttributeError: 'Codebase' object has no attribute 'confirm'
tests/platform_helper/domain/test_codebase.py::test_codebase_deploy_successfully_triggers_a_pipeline_based_deploy
Stack Traces | 0.005s run time
.../platform_helper/domain/test_codebase.py:236: in test_codebase_deploy_successfully_triggers_a_pipeline_based_deploy
    codebase.deploy("test-application", "development", "application", "ab1c23d")
.../dbt_platform_helper/domain/codebase.py:164: in deploy
    self.confirm,
E   AttributeError: 'Codebase' object has no attribute 'confirm'
tests/platform_helper/domain/test_codebase.py::test_codebase_deploy_does_not_trigger_deployment_without_confirmation
Stack Traces | 0.007s run time
.../platform_helper/domain/test_codebase.py:388: in test_codebase_deploy_does_not_trigger_deployment_without_confirmation
    codebase.deploy("test-application", "development", "application", "nonexistent-commit-hash")
.../dbt_platform_helper/domain/codebase.py:164: in deploy
    self.confirm,
E   AttributeError: 'Codebase' object has no attribute 'confirm'

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

@tony-griffin tony-griffin marked this pull request as ready for review December 20, 2024 10:47
@tony-griffin tony-griffin requested a review from a team as a code owner December 20, 2024 10:47
@@ -3,7 +3,7 @@
# Todo: Can we get rid of this yet?
PLATFORM_HELPER_VERSION_FILE = ".platform-helper-version"
# Todo: Move to ???
DEFAULT_TERRAFORM_PLATFORM_MODULES_VERSION = "5"
DEFAULT_TERRAFORM_PLATFORM_MODULES_VERSION = "7"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have version 7 of TPM?

Copy link
Contributor Author

@tony-griffin tony-griffin Dec 20, 2024

Choose a reason for hiding this comment

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

@chiaramapellimt , Version 7 is to be released, that's why this is part of a breaking change.
Version 6 will be the work on the IP filter spoofing that's still waiting to be merged.
Then this PR and John's work on the codebase pipelines will be in version 7.

It's so that we don't over burden services with lots of breaking changes at once I believe, this keeps the new logic seperate.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it a breaking change for platform-tools when we update the main default version of terraform-platform-modules?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it should be actually. It will force users with pipelines to use a later breaking version of t-p-m

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@antroy-madetech Might be better placed to answer that

@@ -433,6 +433,11 @@ def start_build_extraction(codebuild_client, build_options):
return response["build"]["arn"]


def start_pipeline_extraction(codepipeline_client, build_options):
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be "start_pipeline_execution"?

Copy link
Contributor Author

@tony-griffin tony-griffin Dec 20, 2024

Choose a reason for hiding this comment

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

@antroy-madetech It could be as yes we are starting the pipeline execution here. But we are also extracting the execution_id and returning it from this function to create the pipeline execution URL

I followed the existing convention as there is a similar method that extracts the arn from a codebuild response.

I'll leave it you whether you think it shoud be changed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think change it. The important thing that this is doing is starting the pipeline execution. Perhaps "start_pipeline_and_return_execution_id" or something similar would also work. Or just "start_pipeline" and describe what it returns in the docstring?

Copy link
Contributor Author

@tony-griffin tony-griffin Dec 20, 2024

Choose a reason for hiding this comment

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

Renamed to start_pipeline_and_return_execution_id

@@ -397,19 +385,6 @@ def test_codebase_deploy_does_not_trigger_build_with_missing_environment(mock_ap
)


def test_codebase_deploy_does_not_trigger_deployment_without_confirmation():
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we no longer doing confirmation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looked like a duplicate test. There is a test for "without_confirmation" here

Copy link
Contributor

@antroy-madetech antroy-madetech left a comment

Choose a reason for hiding this comment

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

Approved pending the changes suggested

@@ -3,7 +3,7 @@
# Todo: Can we get rid of this yet?
PLATFORM_HELPER_VERSION_FILE = ".platform-helper-version"
# Todo: Move to ???
DEFAULT_TERRAFORM_PLATFORM_MODULES_VERSION = "5"
DEFAULT_TERRAFORM_PLATFORM_MODULES_VERSION = "7"
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it should be actually. It will force users with pipelines to use a later breaking version of t-p-m

@@ -433,6 +433,11 @@ def start_build_extraction(codebuild_client, build_options):
return response["build"]["arn"]


def start_pipeline_extraction(codepipeline_client, build_options):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think change it. The important thing that this is doing is starting the pipeline execution. Perhaps "start_pipeline_and_return_execution_id" or something similar would also work. Or just "start_pipeline" and describe what it returns in the docstring?

@antroy-madetech antroy-madetech deleted the DBTP-1506-update-platform-helper-codebase-deploy-command branch January 27, 2025 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants