Improved execution log for topics and schemas #383
Conversation
Split SyncTopicAction into separate actions for the different parts: create topic, update config and update schema reg
Create a topic config update plan for the UpdateTopicConfigAction. And let the adminClient only update according to the plan.
This reverts commit 00bc474.
Rename methods and ManagerOfThings
Log details on the actual topic config changes that will be done. Also fixed logic for detecting new configs added.
Test logic for detecting new, updated and removed topic configs.
…sable on dind on Windows
…e design around TopologyAclBinding builders, the Action concept and managers for adding actions to the ExecutionPlan.
|
You both rocks! honestly! I am deeply sorry I can answer as fast as I would.
Missatge de Aksel Hilde ***@***.***> del dia dt., 9 de nov.
2021 a les 15:20:
… Thanks Pere! @sverrehu <https://github.com/sverrehu> also have a lot of
contributions in this PR :-)
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#383 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAQXPBAOY4G3DOJ7DMQQ63ULEU2PANCNFSM5HVIVZYA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
--
Pere Urbon-Bayes
Software Architect
https://twitter.com/purbon
https://www.linkedin.com/in/purbon/
|
|
Any progress pon this @purbon ? |
|
Hi,
sorry for not answering earlier. I have taken a look and I really love
what you guys have done here! If you don't mind, I am going to
manually merge this in master (most probably this week), does this make
sense to you?
Sincerely,
…-- Pere
Missatge de Aksel Hilde ***@***.***> del dia dl., 22 de nov.
2021 a les 12:38:
Any progress pon this @purbon <https://github.com/purbon> ?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#383 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAQXPBBIOUIITDEX7JQOM3UNITUTANCNFSM5HVIVZYA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
--
Pere Urbon-Bayes
Software Architect
https://twitter.com/purbon
https://www.linkedin.com/in/purbon/
|
Not sure what you mean by manually merging this into master? |
|
I had to do some releases for work, before merging this, so there are some
conflicts in the PR. This is why I mention manually merging.
Missatge de Aksel Hilde ***@***.***> del dia dl., 22 de nov.
2021 a les 13:16:
… Hi, sorry for not answering earlier. I have taken a look and I really love
what you guys have done here! If you don't mind, I am going to manually
merge this in master (most probably this week), does this make sense to
you? Sincerely,
-- Pere Missatge de Aksel Hilde *@*.***> del dia dl., 22 de nov. 2021 a
les 12:38:
Not sure what you mean by *manually merging* this into master?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#383 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAQXPAON6YRYG5LYAGZ7FLUNIYDJANCNFSM5HVIVZYA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
--
Pere Urbon-Bayes
Software Architect
https://twitter.com/purbon
https://www.linkedin.com/in/purbon/
|
Ok no problem! I would rather not spend more time on handling merge conflicts for this PR, so go ahead! :-) |
|
I knew ;-) and I appreciate any time you has helped this is why I
mentioned!
Keep you posted! thanks a ton again for your help.
…-- Pere
Missatge de Aksel Hilde ***@***.***> del dia dl., 22 de nov.
2021 a les 14:38:
I had to do some releases for work, before merging this, so there are some
conflicts in the PR. This is why I mention manually merging. Missatge de
Aksel Hilde *@*.***> del dia dl., 22 de nov. 2021 a les 13:16:
… <#m_7942461517454096595_>
Ok no problem! I would rather not spend more time on handling merge
conflicts for this PR, so go ahead! :-)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#383 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAQXPEIAKPQFUCPBYFFHOLUNJBVLANCNFSM5HVIVZYA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
--
Pere Urbon-Bayes
Software Architect
https://twitter.com/purbon
https://www.linkedin.com/in/purbon/
|
|
Hi @purbon , any progress on this one? |
|
Manually merging this and making a new version as I write this message. Sorry for the delay, but you know I appreciate all the effort you guys always do! you rock! |
|
Thanks Pere! Great to know that you are now on it. You also rock! With your effort on managing and improving JulieOps! |
|
Hi @akselh I tried rebasing this and solving the conflicts, but it gets easy out of hand. I am doing my best to ease the merge, but it certainly going to take a bit. Sorry for the inconvenience. |
|
@purbon Doing a rebase will probably be very tricky. Maybe I should try a merge from JulieOps master into akselh:execution_log_improvements_2 to check out how bad that will be? I have already been through this once with the merge from julieops/master when I made this PR. |
|
Thanks @akselh, that would be highly appeciated. |
|
I will give it a try today. |
|
@purbon , I managed to merge julieops/master with my branch yesterday - including green tests. But then there are new issues. Today I see that there are new commits to master that requires another round of merge with conflicts... I have looked into this now and pushed a new merge commit. Would be nice to merge this PR before any new commits gets into master? (Pay no attention to my initial message of PR not being updated, it is now anyway). |
|
Hi,
sorry for the new commits, I know it is not nice, but I had to make a new
version for a couple of important reasons, one security (log4j cve) and
another customer work. But you can expect from now on no new commit to
master! I am really sorry for the inconveniences.
For Issue 2, no problem at all! I do understand 100%.
Missatge de Aksel Hilde ***@***.***> del dia dv., 10 de des.
2021 a les 12:15:
… @purbon <https://github.com/purbon> , I managed to merge julieops/master
with my branch yesterday - including green tests. But then there are new
issues:
Issue 1:
Today I see that there are new commits to master that requires another
round of merge with conflicts... I will look into this now, but can you
please await further commits to master today until I get around the new
merge conflicts?
Issue 2:
It does not seem that this PR get the new commits. So probably have to
make a new PR. Or do you have another workaround to get the PR updated with
new commits?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#383 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAQXPBEOHESHON6546VAWTUQHOLZANCNFSM5HVIVZYA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
--
Pere Urbon-Bayes
Software Architect
https://twitter.com/purbon
https://www.linkedin.com/in/purbon/
|
Ok, the merge conflicts was not that hard this time :-) Of course important to handle the log4j CVE. Guess you will also then release new version pretty soon with that fix at least. Regarding PR not updating, not a problem (now at least). |
|
Ok, I saw now that you already made a release today including the log4j upgrade. :-) |
|
@purbon , we are good now I think. What do you think? |
|
Awesome, thanks a lot for the effort! |
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Improvement to execution log output for topics and schemas, see Improve output for topic changes on dryRun and apply #295 for details.
What is the current behavior? (You can also link to an open issue here)
See Improve output for topic changes on dryRun and apply #295
What is the new behavior (if this is a feature change)?
See Improve output for topic changes on dryRun and apply #295.
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
No.
Yes, this is a pretty huge PR, also including some refactoring. I know this is not ideal, but I really cannot see any other way to get these improvements back to JulieOps. The main cause for this is of course the history of this issue (#295); without feedback we had to move on with the work on our fork. Anyway, I hope the contribution is still appreciated!
close #295