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-110989: Adds OnResponseProcessed #298

Merged
merged 3 commits into from May 31, 2022
Merged

CLOUDP-110989: Adds OnResponseProcessed #298

merged 3 commits into from May 31, 2022

Conversation

fmenezes
Copy link
Contributor

@fmenezes fmenezes commented May 27, 2022 •
edited

Description

Please include a summary of the fix/feature/change, including any relevant motivation and context.

Added a OnResponseProcessed for logging purposes

Link to any related issue(s): CLOUDP-110989

Type of change:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Required 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 (if appropriate)
  • I have run make fmt and formatted my code

Further comments

This was referenced May 27, 2022
Merged
Merged
@fmenezes fmenezes marked this pull request as ready for review May 27, 2022 16:28
@fmenezes fmenezes requested a review from a team as a code owner May 27, 2022 16:28
@gssbzn gssbzn added the breaking Pull requests that breaks backwards compatibility label May 27, 2022
gssbzn
gssbzn reviewed May 27, 2022
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.

Given you are changing an exported function this is a breaking change, could we make this a non breaking change?

The other thing, I'm finding having both onRequestCompleted and onAfterRequestCompleted confusing, is there a better name for onAfterRequestCompleted? onResponseProcessed or similar?

@fmenezes
Copy link
Contributor Author

@gssbzn I'll make it a non breaking change before finishing up

@fmenezes
Copy link
Contributor Author

@gssbzn as mentioned offline this change will most likely be more beneficial to keep on a breaking change

@fmenezes
Copy link
Contributor Author

@themantissa please tag terraform team as reviewers

@fmenezes fmenezes changed the title CLOUDP-110989: Adds OnAfterRequestCompleted CLOUDP-110989: Adds OnResponseProcessed May 30, 2022
gssbzn
gssbzn approved these changes May 30, 2022
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.

LGTM

evertsd
evertsd approved these changes May 31, 2022
View reviewed changes
Copy link
Collaborator

@evertsd evertsd left a comment •
edited

Choose a reason for hiding this comment

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

LGTM, does not look like this breaking change will affect (break) terraform-provider-mongodbatlas

@fmenezes fmenezes merged commit 551edbf into master May 31, 2022
@fmenezes fmenezes deleted the CLOUDP-110989 branch May 31, 2022 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gssbzn gssbzn gssbzn approved these changes

@evertsd evertsd evertsd approved these changes

@martinstibbe martinstibbe Awaiting requested review from martinstibbe

Assignees
No one assigned
Labels
breaking Pull requests that breaks backwards compatibility
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@fmenezes @gssbzn @evertsd