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

MGDCTRS-537 Add processor definitions for Debezium SMTs/processors #28

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

MGDCTRS-537 Add processor definitions for Debezium SMTs/processors #28

wants to merge 4 commits into from

Conversation

rk3rn3r
Copy link
Contributor

@rk3rn3r rk3rn3r commented Jan 11, 2023

@rk3rn3r rk3rn3r requested review from Naros and ani-sha January 11, 2023 20:08
@rk3rn3r
Copy link
Contributor Author

rk3rn3r commented Jan 11, 2023 •
edited
Loading

@roldanbob you maybe want to have a look at the description texts and their wordings?

@rk3rn3r rk3rn3r force-pushed the MGDCTRS-535-add-processors-to-catalog branch from abd5e5e to 21e2615 Compare January 11, 2023 20:11
@roldanbob
Copy link

@roldanbob you might want to have a look at the description texts and their wordings?

Will do, @rk3rn3r. Thanks for the nod.

@rk3rn3r
Copy link
Contributor Author

rk3rn3r commented Jan 11, 2023 •
edited
Loading

Open todos:

  • adding missing "title" and "description" fields
  • reviewing and fixing wordings

roldanbob
roldanbob suggested changes Jan 12, 2023
View reviewed changes
Copy link

@roldanbob roldanbob left a comment

Choose a reason for hiding this comment

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

@rk3rn3r As I noted in one of the in-line comments, some of my edits are best guesses, since I don't fully grasp how some of the regex entries are employed. I'm available to meet if you want to share further details.

src/main/resources/additional_definitions.json Outdated Show resolved Hide resolved
src/main/resources/additional_properties.json Outdated Show resolved Hide resolved
src/main/resources/additional_properties.json Outdated Show resolved Hide resolved
src/main/resources/additional_properties.json Outdated Show resolved Hide resolved
src/main/resources/additional_properties.json Outdated Show resolved Hide resolved
src/main/resources/additional_properties.json Outdated Show resolved Hide resolved
src/main/resources/additional_properties.json Outdated Show resolved Hide resolved
src/main/resources/additional_properties.json Outdated Show resolved Hide resolved
src/main/resources/additional_properties.json Outdated Show resolved Hide resolved
src/main/resources/additional_properties.json Show resolved Hide resolved
@rk3rn3r rk3rn3r force-pushed the MGDCTRS-535-add-processors-to-catalog branch 3 times, most recently from 1a7c43f to 15d99ea Compare February 2, 2023 13:36
@rk3rn3r rk3rn3r force-pushed the MGDCTRS-535-add-processors-to-catalog branch from 15d99ea to c4162d5 Compare February 2, 2023 14:07
@rk3rn3r rk3rn3r marked this pull request as ready for review February 2, 2023 14:26
@rk3rn3r
Copy link
Contributor Author

rk3rn3r commented Feb 2, 2023 •
edited
Loading

@roldanbob thank you for the initial review. I finished the work on the PR and applied parts of your suggested changes.
I was not able to apply all your changes as they might be a bit off from what is happening exactly. Especially there seems to be some confusion about the regular expressions and how to name the regex replacement string...

I have pushed a separate commit with the docs changes that you requested. Would be great if @Naros could have a look at this too?
@Naros, maybe you could have a look in at the description texts, the changes in that commit, and at the PR in general?

@roldanbob
Copy link

@roldanbob thank you for the initial review. I finished the work on the PR and applied parts of your suggested changes. ... I have pushed a separate commit with the docs changes that you requested. Would be great if @Naros could have a look at this too?

Thanks, @rk3rn3r! I'll take another look!

@rk3rn3r rk3rn3r force-pushed the MGDCTRS-535-add-processors-to-catalog branch from e0fc7c6 to 67ab8d6 Compare February 8, 2023 14:34
MelissaFlinn
MelissaFlinn reviewed Feb 13, 2023
View reviewed changes
Copy link

@MelissaFlinn MelissaFlinn left a comment

Choose a reason for hiding this comment

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

Hi René - here are some suggestions. Let me know if you'd like me to continue

descriptors/mongodb/debezium-mongodb-2.0.1.Final.json Outdated Show resolved Hide resolved
descriptors/mongodb/debezium-mongodb-2.0.1.Final.json Outdated Show resolved Hide resolved
descriptors/mongodb/debezium-mongodb-2.0.1.Final.json Outdated Show resolved Hide resolved
descriptors/mongodb/debezium-mongodb-2.0.1.Final.json Outdated Show resolved Hide resolved
descriptors/mongodb/debezium-mongodb-2.0.1.Final.json Outdated Show resolved Hide resolved
descriptors/mongodb/debezium-mongodb-2.0.1.Final.json Outdated Show resolved Hide resolved
descriptors/mongodb/debezium-mongodb-2.0.1.Final.json Outdated Show resolved Hide resolved
descriptors/mongodb/debezium-mongodb-2.0.1.Final.json Outdated Show resolved Hide resolved
descriptors/mongodb/debezium-mongodb-2.0.1.Final.json Outdated Show resolved Hide resolved
descriptors/mongodb/debezium-mongodb-2.0.1.Final.json Outdated Show resolved Hide resolved
@rk3rn3r
Copy link
Contributor Author

rk3rn3r commented Feb 22, 2023 •
edited
Loading

@MelissaFlinn Thank you for your valuable feedback! 🙏
I applied most of your suggestions and commented on the ones where I was not sure.

Edit: Looks like I missed some comments that were collapsed and not visible. I got back to them now.
Edit#2: I have added my changes related to Melissa's review as a separate commit and will squash all commits when merging.

@rk3rn3r rk3rn3r force-pushed the MGDCTRS-535-add-processors-to-catalog branch from 05b2a50 to ffa3cd1 Compare February 28, 2023 11:37
@rk3rn3r rk3rn3r force-pushed the MGDCTRS-535-add-processors-to-catalog branch from ffa3cd1 to 29764f3 Compare March 30, 2023 09:21
@rk3rn3r rk3rn3r force-pushed the MGDCTRS-535-add-processors-to-catalog branch from 29764f3 to 0a8b0d3 Compare April 5, 2023 08:23
@rk3rn3r rk3rn3r mentioned this pull request Apr 6, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MelissaFlinn MelissaFlinn MelissaFlinn left review comments

@roldanbob roldanbob roldanbob requested changes

@Naros Naros Awaiting requested review from Naros

@ani-sha ani-sha Awaiting requested review from ani-sha

Assignees
No one assigned
Labels
don't merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rk3rn3r @roldanbob @Naros @MelissaFlinn