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

CLOUDP-205482: Remove legacy deployment #1182

Merged
merged 8 commits into from
Nov 2, 2023

Conversation

igor-karpukhin
Copy link
Collaborator

@igor-karpukhin igor-karpukhin commented Oct 19, 2023

Changes

  • Removed spec.advancedDeploymentSpec from the AtlasDeployment resource.
  • spec.deploymentSpec now contains fields from spec.advancedDeploymentSpec
  • Fixed E2E pipeline so now it will regenerate CRDs from go files before running E2E tests.

All Submissions:

  • Have you signed our CLA?
  • Put closes #XXXX in your comment to auto-close the issue that your PR fixes (if there is one).
  • Update docs/release-notes/release-notes.md if your changes should be included in the release notes for the next release.

@igor-karpukhin igor-karpukhin force-pushed the CLOUDP-205482/remove-legacy-deployment branch 2 times, most recently from a8b1b38 to 73120ed Compare October 23, 2023 13:36
@igor-karpukhin igor-karpukhin force-pushed the CLOUDP-205482/remove-legacy-deployment branch 2 times, most recently from 2d2767c to c8ff524 Compare October 27, 2023 20:05
@igor-karpukhin igor-karpukhin force-pushed the CLOUDP-205482/remove-legacy-deployment branch from c8ff524 to fb2b806 Compare October 27, 2023 20:09
@igor-karpukhin igor-karpukhin marked this pull request as ready for review October 27, 2023 20:15
@github-actions
Copy link
Contributor

github-actions bot commented Oct 27, 2023

@@ -86,14 +77,7 @@ func deploymentForGov(deployment *mdbv1.AtlasDeploymentSpec, regionUsageRestrict
var err error

if deployment.DeploymentSpec != nil {
regionErr := validCloudGovRegion(regionUsageRestrictions, deployment.DeploymentSpec.ProviderSettings.RegionName)
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure why this gov related check is no longer needed here, can you elaborate?

Copy link
Collaborator Author

@igor-karpukhin igor-karpukhin Oct 30, 2023

Choose a reason for hiding this comment

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

Since there is no more legacy clusters, regions need to be validated for every replica set. See this method is called below.

@@ -40,4 +40,4 @@ export MCLI_PUBLIC_API_KEY="${public_key}"
export MCLI_PRIVATE_API_KEY="${private_key}"
export MCLI_ORG_ID="${org_id}"
export IMAGE_URL="${image}" #for helm chart
AKO_E2E_TEST=1 ginkgo --label-filter="${focus_key}" --timeout 120m -v test/e2e/
AKO_E2E_TEST=1 ginkgo --label-filter="${focus_key}" --timeout 120m -vv test/e2e/
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need this verbosity in the final version?

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 gives a better output for the errors. And this is only for the local usage as you can see

test/e2e/actions/steps.go Outdated Show resolved Hide resolved
test/e2e/actions/steps.go Outdated Show resolved Hide resolved
@josvazg
Copy link
Collaborator

josvazg commented Oct 30, 2023

I suspect the helm update issue might not be transient?

Copy link
Collaborator

@helderjs helderjs left a comment

Choose a reason for hiding this comment

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

Good overall! Massive work as usual. Just some clean up required

deploy/crds/atlas.mongodb.com_atlasdeployments.yaml Outdated Show resolved Hide resolved
pkg/controller/atlasdeployment/deployment_test.go Outdated Show resolved Hide resolved
pkg/controller/atlasdeployment/deployment_test.go Outdated Show resolved Hide resolved
pkg/controller/atlasdeployment/deployment_test.go Outdated Show resolved Hide resolved
pkg/controller/validate/validate_test.go Outdated Show resolved Hide resolved
test/e2e/actions/deploy/deploy_operator.go Outdated Show resolved Hide resolved
test/e2e/actions/steps.go Outdated Show resolved Hide resolved
test/e2e/actions/steps.go Outdated Show resolved Hide resolved
test/e2e/actions/steps.go Outdated Show resolved Hide resolved
@igor-karpukhin
Copy link
Collaborator Author

@josvazg @helderjs thanks for your comments! I resolved all of them I think. As for the helm-update, I'm looking into it now

@helderjs
Copy link
Collaborator

@josvazg @helderjs thanks for your comments! I resolved all of them I think. As for the helm-update, I'm looking into it now

Looks good, I believe there's a last left over to be cleaned but once the test is green it's good from my side

@josvazg
Copy link
Collaborator

josvazg commented Nov 2, 2023

LGTM, waiting for testing to go green to approve

@igor-karpukhin
Copy link
Collaborator Author

As per our conversation today, shall we proceed and merge this PR? @helderjs @josvazg

Copy link
Collaborator

@josvazg josvazg left a comment

Choose a reason for hiding this comment

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

LGTM, lets proceed

@igor-karpukhin igor-karpukhin merged commit 8676519 into main Nov 2, 2023
40 of 41 checks passed
@igor-karpukhin igor-karpukhin deleted the CLOUDP-205482/remove-legacy-deployment branch November 2, 2023 14:11
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.

3 participants