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

Kafka V1 Support #8381

Merged
merged 15 commits into from
Dec 13, 2024
Merged

Kafka V1 Support #8381

merged 15 commits into from
Dec 13, 2024

Conversation

jamarcelin
Copy link
Contributor

No description provided.

@jamarcelin
Copy link
Contributor Author

jamarcelin commented Dec 8, 2024

@bblommers I am having some issue with some of the endpoints. On line 147 of moto/kafka/responses.py, it is correctly printing (and returning) the response but the response isn't correct in the tests. I think it's a URL issues but not sure. Any ideas?

@bblommers
Copy link
Collaborator

Hi @jamarcelin! It looks like some of the response-keys simply need to be lowercased. For example:

diff --git a/moto/kafka/models.py b/moto/kafka/models.py
index 5cd36741d..e2973d5f8 100644
--- a/moto/kafka/models.py
+++ b/moto/kafka/models.py
@@ -231,11 +231,11 @@ class KafkaBackend(BaseBackend):
     ):
         cluster_info_list = [
             {
-                "ClusterArn": cluster.arn,
-                "ClusterName": cluster.cluster_name,
-                "ClusterType": cluster.cluster_type,
-                "State": cluster.state,
-                "CreationTime": cluster.creation_time,
+                "clusterArn": cluster.arn,
+                "clusterName": cluster.cluster_name,
+                "clusterType": cluster.cluster_type,
+                "state": cluster.state,
+                "creationTime": cluster.creation_time,
             }
             for cluster in self.clusters.values()
         ]

That change ensures that the list_clusters_v2()-method call succeeds in the test_create_cluster_v2(). I imagine that the other failures/tests have similar problems/solutions, but I haven't tested that.

This is the first thing I tried, and it happened to work - just because I have run into this problem in the past. The botocore spec is always good place to confirm things like this though, as you can see that they lowercase all locationName's:

https://github.com/boto/botocore/blob/cacd0e9fdba71d1f976069a072576ad74b888bea/botocore/data/kafka/2018-11-14/service-2.json#L2429

@jamarcelin
Copy link
Contributor Author

@bblommers Gotcha, thanks for the help! I fixed the tests and made some changes. The linter is having an issue with broker_node_group_info from FakeKafkaCluster. Any idea how to resolve this?

@bblommers
Copy link
Collaborator

Hi @jamarcelin! You can run ruff format moto/kafka to fix that. I've pushed a fix for now + some typing fixes - I'll leave the test failures to you 🙂

@jamarcelin
Copy link
Contributor Author

@bblommers Test should be fixed now, just waiting on check to complete. Thanks again!

@jamarcelin
Copy link
Contributor Author

@bblommers Hey, sorry to keep pinging, it seems the tests are failing in server mode? Is this caused by an issue with the urls?

ARNs contain a forward slash
Copy link

codecov bot commented Dec 10, 2024

Codecov Report

Attention: Patch coverage is 97.29730% with 5 lines in your changes missing coverage. Please review.

Project coverage is 94.60%. Comparing base (1291c55) to head (5a3e308).
Report is 13 commits behind head on master.

Files with missing lines Patch % Lines
moto/resourcegroupstaggingapi/models.py 54.54% 5 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           master    #8381    +/-   ##
========================================
  Coverage   94.59%   94.60%            
========================================
  Files        1159     1163     +4     
  Lines      101394   101632   +238     
========================================
+ Hits        95911    96144   +233     
- Misses       5483     5488     +5     
Flag Coverage Δ
servertests 28.84% <18.91%> (-0.03%) ⬇️
unittests 94.57% <97.29%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@bpandola
Copy link
Collaborator

@jamarcelin I pushed a commit to fix the urls. They have ARNs in them, which contain forward slashes.

@jamarcelin
Copy link
Contributor Author

jamarcelin commented Dec 10, 2024

Awesome! Thanks for the help! This looks good to merge to me. Also, is there a specific cadence for releases? Or is there anyway to trigger a release?

Copy link
Collaborator

@bpandola bpandola left a comment

Choose a reason for hiding this comment

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

Can you add tests to cover the lines flagged by Codecov? It's unclear, for example, whether or not FakeKafkaCluster.to_dict() is unreachable code or just not covered by tests.

@jamarcelin jamarcelin requested a review from bpandola December 11, 2024 20:21
partition = get_partition(self.region_name)
return f"arn:{partition}:kafka:{self.region_name}:{self.account_id}:{resource_type}/{self.cluster_id}"

def to_dict(self) -> Dict[str, Any]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we not remove this method altogether @jamarcelin? As far as I can tell, it's not used anywhere

@bblommers
Copy link
Collaborator

Also, is there a specific cadence for releases? Or is there anyway to trigger a release?

I try to do them every two weeks - the next one should come this weekend. 🙂

@bblommers bblommers added this to the 5.0.23 milestone Dec 13, 2024
Copy link
Collaborator

@bblommers bblommers left a comment

Choose a reason for hiding this comment

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

Great - thank you @jamarcelin!

@bblommers bblommers merged commit 44df2b0 into getmoto:master Dec 13, 2024
52 of 53 checks passed
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.

3 participants