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

Add Kafka ACL admin to website #2301

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

Conversation

dakota002
Copy link
Contributor

@dakota002 dakota002 commented May 24, 2024

Description

New route to manage a DynamoDB representation of the ACLs defined on the Kafka brokers

@dakota002 dakota002 force-pushed the AdminKafka branch 3 times, most recently from a64936c to 2f2ca44 Compare June 4, 2024 15:51
@dakota002 dakota002 force-pushed the AdminKafka branch 3 times, most recently from 4384405 to 79e8fd5 Compare July 16, 2024 15:16
@dakota002 dakota002 requested review from lpsinger and Courey and removed request for lpsinger August 5, 2024 17:16
@dakota002 dakota002 marked this pull request as ready for review August 5, 2024 17:16
@dakota002
Copy link
Contributor Author

I am marking this as ready for review, but I know it will need a semi-hefty refactor after #2268 gets merged in

@dakota002 dakota002 self-assigned this Aug 5, 2024
@dakota002 dakota002 changed the title WIP: Add Kafka ACL admin to website Add Kafka ACL admin to website Aug 5, 2024
@dakota002 dakota002 force-pushed the AdminKafka branch 3 times, most recently from 5681ba0 to 59e78c2 Compare August 7, 2024 19:03
@lpsinger
Copy link
Member

lpsinger commented Aug 7, 2024

Heads up that it is going to take me a while to get to reviewing this. This is going to be a challenging review because it has more security and data consistency impact than typical PRs.

@dakota002
Copy link
Contributor Author

Heads up that it is going to take me a while to get to reviewing this. This is going to be a challenging review because it has more security and data consistency impact than typical PRs.

No worries, I stripped out some of the changes that were starting to touch on the NoticeTypeCheckboxes to a separate branch to try to simplify this one a bit more

Copy link

codecov bot commented Sep 12, 2024

Codecov Report

Attention: Patch coverage is 0% with 97 lines in your changes missing coverage. Please review.

Project coverage is 6.08%. Comparing base (95edeb6) to head (1791830).

Files with missing lines Patch % Lines
app/routes/admin.kafka._index.tsx 0.00% 46 Missing ⚠️
app/lib/kafka.server.ts 0.00% 44 Missing ⚠️
app/root.tsx 0.00% 5 Missing ⚠️
app/root/header/Header.tsx 0.00% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##            main   #2301      +/-   ##
========================================
- Coverage   6.21%   6.08%   -0.14%     
========================================
  Files        167     168       +1     
  Lines       4245    4341      +96     
  Branches     471     478       +7     
========================================
  Hits         264     264              
- Misses      3979    4075      +96     
  Partials       2       2              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

Copy link
Member

@lpsinger lpsinger left a comment

Choose a reason for hiding this comment

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

I am not entirely content with having buttons to manually synchronize ACLs from the Kafka cluster to DynamoDB records or vice versa.

This will require some more thought about the impacts of changes to ACLS that are made by other means than through the web application --- for example, changes made through the Confluent Control Center.

@dakota002
Copy link
Contributor Author

I am not entirely content with having buttons to manually synchronize ACLs from the Kafka cluster to DynamoDB records or vice versa.

This will require some more thought about the impacts of changes to ACLS that are made by other means than through the web application --- for example, changes made through the Confluent Control Center.

Yea, I discussed this earlier in the meeting, but this PR needs to be majorly refactored. I'm going to mark it as a draft in the meantime

@dakota002 dakota002 marked this pull request as draft October 16, 2024 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

3 participants