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

fix(rds): reset parameters removed from a parametergroup #1712

Merged
merged 1 commit into from
Apr 24, 2023
Merged

fix(rds): reset parameters removed from a parametergroup #1712

merged 1 commit into from
Apr 24, 2023

Conversation

AlexLast
Copy link
Contributor

Description of your changes

The existing update logic for DBClusterParameterGroup will only update parameters defined in spec.forProvider.parameters, if a parameter is removed it will no longer be managed.

This contribution ensures parameters removed from spec.forProvider.parameters will be reset to their default value. Assuming this approach is ok, I'll raise a similar PR for DBParameterGroup.

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

  • Running a local build, testing existing and new functionality is working as expected

@AlexLast
Copy link
Contributor Author

Looks like some changes have been made in this area in #1709, I'll rebase and refactor slightly.

@AlexLast
Copy link
Contributor Author

@haarchri @MisterMX would you mind reviewing?

@MisterMX
Copy link
Collaborator

@AlexLast does this actually reset parameters that have been removed from the spec? There are changes in the update logic. Don't you have set the original values their as weel?

Also, can you implement the changes for DBParameterGroup as well?

@AlexLast
Copy link
Contributor Author

AlexLast commented Apr 11, 2023

@MisterMX I refactored slightly after the change to the update logic, I have rebased and manually tested, it is still working as expected. This will reset any parameters that are removed from spec, when using the ResetDBClusterParameterGroup call original values are not required. See - https://docs.aws.amazon.com/AmazonRDS/latest/APIReference/API_ResetDBClusterParameterGroup.html

Have updated PR to include the same logic for DBParameterGroup.

@AlexLast AlexLast changed the title fix(rds): reset parameters removed from a dbclusterparametergroup fix(rds): reset parameters removed from a parametergroup Apr 15, 2023
Signed-off-by: Alex Last <alexrichardlast@gmail.com>
@AlexLast
Copy link
Contributor Author

Hey @MisterMX @haarchri apologies to tag you again, would it be possible to review? Would ideally like to get this in before next release 🤞

@MisterMX
Copy link
Collaborator

Sorry, @AlexLast for the late response. Unfortunately, I couldn't make it in time for v0.39.

Howerver, this is looking good to me now. Thanks a lot!

@MisterMX MisterMX merged commit a62fc28 into crossplane-contrib:master Apr 24, 2023
@AlexLast
Copy link
Contributor Author

No problem, thanks for the review 👍🏻

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.

None yet

2 participants