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

Remove all dependencies from the migration-console stack #1043

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gregschohn
Copy link
Collaborator

That means that the console can come up and be used for diagnostics if the other stacks/clusters don't properly deploy.

Description

This change allows the migration console to be deployed w/out any dependencies of other tasks that it's meant to interface with so that the console itself can be used as a diagnostic tool

  • Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation)
  • Why these changes are required?
  • What is the old behavior before changes and new behavior after changes?

Issues Resolved

https://opensearch.atlassian.net/browse/MIGRATIONS-2037

Is this a backport? If so, please add backport PR # and/or commits #

Testing

limited testing by redploying from my bootstrap box

Check List

  • New functionality includes testing
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Copy link

codecov bot commented Oct 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.57%. Comparing base (f28e131) to head (c332e1e).
Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #1043   +/-   ##
=========================================
  Coverage     80.56%   80.57%           
  Complexity     2736     2736           
=========================================
  Files           365      365           
  Lines         13611    13611           
  Branches        941      941           
=========================================
+ Hits          10966    10967    +1     
  Misses         2067     2067           
+ Partials        578      577    -1     
Flag Coverage Δ
gradle-test 78.61% <ø> (+<0.01%) ⬆️
python-test 90.11% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@AndreKurait AndreKurait left a comment

Choose a reason for hiding this comment

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

I'm fine with the spirit of this change, though i'm a bit concerned with how this interacts with the SSM parameters dependencies since they would be added during deployment.

Have we tested with this PR on the first deployment in a new stage with concurrency set to a very high value?

Copy link
Member

@AndreKurait AndreKurait left a comment

Choose a reason for hiding this comment

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

Please verify ^ test case before merge

Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

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

I'm fine with the spirit of this change, though i'm a bit concerned with how this interacts with the SSM parameters dependencies since they would be added during deployment.

Have we tested with this PR on the first deployment in a new stage with concurrency set to a very high value?

@AndreKurait does the jenkins job verify this behavior so manually checks aren't needed for this change?

@@ -621,8 +621,7 @@ export class StackComposer {
})
// To enable the Migration Console to make requests to other service endpoints with services,
// it must be deployed after any connected services
this.addDependentStacks(migrationConsoleStack, [captureProxyESStack, captureProxyStack, elasticsearchStack,
openSearchStack, osContainerStack, migrationStack, kafkaBrokerStack])
this.addDependentStacks(migrationConsoleStack, [migrationStack])
Copy link
Member

Choose a reason for hiding this comment

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

This seems like the correct list, I think we accidentally inverted the dependencies when this was first added.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can remove the comment above this as well. Originally when we used service connect to talk between ECS services, the Migration Console would need to be deployed after those services to have their service name mapping with service connect. We no longer user Service Connect I believe so that shouldn't block us.

The other concern I have is loading in environment variables. If we are still loading in environment variables from other stacks that create a source or target (not our import case) say in the opensearch container stack case, we wanted to be sure that endpoint existed and could be passed. This seems like something CDK may be smart enough to detect without manually doing it, but it would be nice to confirm that's the case if we still want to support that use case.

That means that the console can come up and be used for diagnostics if the other stacks/clusters don't properly deploy.

Signed-off-by: Greg Schohn <[email protected]>
@gregschohn gregschohn force-pushed the RemoveSourceAndProxyDependenciesFromMigrationConsoleCDK branch from c5c2376 to c332e1e Compare October 3, 2024 20:33
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.

4 participants