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.

Sign up for GitHub

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

Jump to bottom

CLOUDP-212065: fix: add 2.0.0 major version of AKO #2459

Merged
merged 4 commits into from
Nov 30, 2023
Merged

CLOUDP-212065: fix: add 2.0.0 major version of AKO #2459

merged 4 commits into from
Nov 30, 2023

Conversation

s-urbaniak
Copy link
Collaborator

@s-urbaniak s-urbaniak commented Nov 20, 2023 •
edited
Loading

Proposed changes

Starting from the changes present in this PR, rendering AKO v1.x resources is not supported any more. Starting from here, rendering AKO v2.x resources is possible only.

Jira ticket: CLOUDP-210683

Checklist

  • I have signed the MongoDB CLA
  • I have added tests that prove my fix is effective or that my feature works
  • I have added any necessary documentation in document requirements section listed in CONTRIBUTING.md (if appropriate)
  • I have addressed the @mongodb/docs-cloud-team comments (if appropriate)
  • I have updated test/README.md (if an e2e test has been added)
  • I have run make fmt and formatted my code

Further comments

@s-urbaniak s-urbaniak requested a review from a team November 20, 2023 14:12
@s-urbaniak s-urbaniak changed the title Cloudp 210683 CLOUDP-210683: fix: add 2.0.0 major version of AKO Nov 20, 2023
@s-urbaniak s-urbaniak mentioned this pull request Nov 20, 2023
Closed
6 tasks
@s-urbaniak s-urbaniak requested a review from a team as a code owner November 20, 2023 14:15
@s-urbaniak s-urbaniak force-pushed the CLOUDP-210683 branch 2 times, most recently from ecfe6e1 to 90c8768 Compare November 20, 2023 14:17
@s-urbaniak
Copy link
Collaborator Author

@mongodb/docs-cloud-team please be aware of the changes mentioned in the PR title. Please let me know if you have further questions, thank you!

@s-urbaniak s-urbaniak force-pushed the CLOUDP-210683 branch 6 times, most recently from 481b06d to 674f017 Compare November 22, 2023 15:04
@s-urbaniak s-urbaniak changed the title CLOUDP-210683: fix: add 2.0.0 major version of AKO CLOUDP-212065: fix: add 2.0.0 major version of AKO Nov 24, 2023
@s-urbaniak s-urbaniak force-pushed the CLOUDP-210683 branch 7 times, most recently from 20a4e19 to 92e52c2 Compare November 24, 2023 16:01
gssbzn
gssbzn reviewed Nov 24, 2023
View reviewed changes
test/e2e/atlas/helper_test.go Outdated Show resolved Hide resolved
go.mod Show resolved Hide resolved
test/e2e/atlas/operator_helper_test.go Outdated Show resolved Hide resolved
test/e2e/atlas/kubernetes_config_apply_test.go Show resolved Hide resolved
igor-karpukhin
igor-karpukhin reviewed Nov 27, 2023
View reviewed changes
Copy link
Contributor

@igor-karpukhin igor-karpukhin left a comment

Choose a reason for hiding this comment

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

There is one issue with the type check. LGTM in general, if the CI is green.

gssbzn
gssbzn reviewed Nov 27, 2023
View reviewed changes
internal/kubernetes/operator/deployment/deployment_test.go
akov2common "github.com/mongodb/mongodb-atlas-kubernetes/v2/pkg/api/v1/common"
akov2provider "github.com/mongodb/mongodb-atlas-kubernetes/v2/pkg/api/v1/provider"
akov2status "github.com/mongodb/mongodb-atlas-kubernetes/v2/pkg/api/v1/status"
akov2toptr "github.com/mongodb/mongodb-atlas-kubernetes/v2/pkg/util/toptr"
Copy link
Collaborator

Choose a reason for hiding this comment

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

👀

gssbzn
gssbzn reviewed Nov 27, 2023
View reviewed changes
internal/kubernetes/operator/features/crds.go
ResourceAtlasDataFederation,
},
"1.9.0": {
"2.0.0": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

[q] we are dropping support for v1 then? I'm ok just confirming

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

to be more precise: we are not dropping support for v1, just newer versions of Atlas CLI won't be able to generate v1.x manifests.

@s-urbaniak s-urbaniak force-pushed the CLOUDP-210683 branch 2 times, most recently from 74ccf55 to 6cd015b Compare November 28, 2023 09:06
@s-urbaniak s-urbaniak changed the title CLOUDP-212065: fix: add 2.0.0 major version of AKO WIP CLOUDP-212065: fix: add 2.0.0 major version of AKO Nov 28, 2023
@s-urbaniak
Copy link
Collaborator Author

s-urbaniak commented Nov 28, 2023 •
edited
Loading

@gssbzn i hopefully addressed all your concerns, thanks for the feedback, PTAL. Having said that I prefixed this PR as WIP (work in progress). We unfortunately found another bug yesterday in AKO v2.0 which we are fixing. The implied changes here most likely will be:

  • We will not advertise 2.0.0 in the Atlas-CLI, just the 2.0.1 release including the bug fixes
  • reference the 2.0.1 images

So my suggestion is to merge this not sooner than we release AKO 2.0.1.

cc @igor-karpukhin

@s-urbaniak s-urbaniak changed the title WIP CLOUDP-212065: fix: add 2.0.0 major version of AKO CLOUDP-212065: fix: add 2.0.0 major version of AKO Nov 28, 2023
@s-urbaniak
Copy link
Collaborator Author

ok, I learned that we always pull the latest image, so this is good for final review.

gssbzn
gssbzn previously approved these changes Nov 28, 2023
View reviewed changes
Copy link
Collaborator

@gssbzn gssbzn left a comment

Choose a reason for hiding this comment

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

Changes LGTM you can decide when is best to merge this once the operator fixes are out but I'm not blocking the PR

igor-karpukhin
igor-karpukhin previously approved these changes Nov 28, 2023
View reviewed changes
Copy link
Contributor

@igor-karpukhin igor-karpukhin left a comment

Choose a reason for hiding this comment

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

Great work! 👍

@s-urbaniak s-urbaniak changed the title CLOUDP-212065: fix: add 2.0.0 major version of AKO WIP CLOUDP-212065: fix: add 2.0.0 major version of AKO Nov 29, 2023
@s-urbaniak
Copy link
Collaborator Author

before we merge, I'd like to implement one last optimization. Currently, we always generate empty secret templates. That's not really necessary unless a concrete alert integration is configured in Atlas.

We can consider the integration type name to see if we actually need to create a secret template.

@s-urbaniak s-urbaniak force-pushed the CLOUDP-210683 branch 4 times, most recently from ff94b50 to c9512dc Compare November 29, 2023 13:31
@s-urbaniak s-urbaniak changed the title WIP CLOUDP-212065: fix: add 2.0.0 major version of AKO \CLOUDP-212065: fix: add 2.0.0 major version of AKO Nov 29, 2023
@s-urbaniak s-urbaniak changed the title \CLOUDP-212065: fix: add 2.0.0 major version of AKO CLOUDP-212065: fix: add 2.0.0 major version of AKO Nov 29, 2023
igor-karpukhin
igor-karpukhin previously approved these changes Nov 29, 2023
View reviewed changes
Copy link
Contributor

@igor-karpukhin igor-karpukhin left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@s-urbaniak s-urbaniak merged commit c67c5c4 into master Nov 30, 2023
15 of 17 checks passed
@s-urbaniak s-urbaniak deleted the CLOUDP-210683 branch November 30, 2023 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@igor-karpukhin igor-karpukhin igor-karpukhin approved these changes

@gssbzn gssbzn gssbzn left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@s-urbaniak @gssbzn @igor-karpukhin