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

feat: Support for self managed kafka as an event source #2091

Merged
merged 14 commits into from
Oct 8, 2021

Conversation

jonife
Copy link
Contributor

@jonife jonife commented Jul 14, 2021

Issue #, if available:

Description of changes:
To provide supporting SelfManagedKafka as an event source type in SAM and also provide support for SASL/PLAIN (BASIC_AUTH) as an Auth mechanism.

Description of how you validated changes:
Wrote a couple of tests

Checklist:

  • Write/update tests
  • make pr passes
  • Update documentation

@mgrandis mgrandis changed the title SAM support for self managed kafka as an event source feat: Support for self managed kafka as an event source Jul 14, 2021
Copy link
Contributor

@mgrandis mgrandis left a comment

Choose a reason for hiding this comment

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

Added a few comments

samtranslator/model/eventsources/pull.py Outdated Show resolved Hide resolved
samtranslator/model/eventsources/pull.py Outdated Show resolved Hide resolved
samtranslator/model/eventsources/pull.py Outdated Show resolved Hide resolved
samtranslator/model/eventsources/pull.py Outdated Show resolved Hide resolved
samtranslator/model/eventsources/pull.py Outdated Show resolved Hide resolved
samtranslator/model/eventsources/pull.py Outdated Show resolved Hide resolved
Copy link
Contributor

@mgrandis mgrandis left a comment

Choose a reason for hiding this comment

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

Thanks for the extensive test additions!
I think we also want to add:

  • unit tests with intrinsic functions
  • 1 integration test without intrinsic functions
  • 1 integration test with intrinsic functions

We are working on adding tests to other events/resources I think this is needed.
See:

samtranslator/model/eventsources/pull.py Outdated Show resolved Hide resolved
@mgrandis
Copy link
Contributor

Thanks for the intrinsic tests!
We still need to see if IP addresses are allowed/supported in KafkaBootstrapServers.

@mgrandis
Copy link
Contributor

The AppVeyor run failed because you added the IP address to the input test templates and not the output ones

@codecov-commenter
Copy link

codecov-commenter commented Jul 30, 2021

Codecov Report

Merging #2091 (8ee3d81) into develop (e7a1496) will increase coverage by 0.75%.
The diff coverage is 98.19%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2091      +/-   ##
===========================================
+ Coverage    93.58%   94.33%   +0.75%     
===========================================
  Files           90       95       +5     
  Lines         6124     6495     +371     
  Branches      1260     1306      +46     
===========================================
+ Hits          5731     6127     +396     
+ Misses         183      170      -13     
+ Partials       210      198      -12     
Impacted Files Coverage Δ
samtranslator/model/lambda_.py 93.10% <ø> (ø)
samtranslator/plugins/globals/globals.py 99.05% <ø> (ø)
samtranslator/region_configuration.py 77.77% <63.63%> (-22.23%) ⬇️
samtranslator/translator/translator.py 97.26% <90.00%> (+0.07%) ⬆️
samtranslator/swagger/swagger.py 93.65% <91.89%> (+0.28%) ⬆️
samtranslator/__init__.py 100.00% <100.00%> (ø)
samtranslator/feature_toggle/dialup.py 100.00% <100.00%> (ø)
samtranslator/feature_toggle/feature_toggle.py 100.00% <100.00%> (+12.16%) ⬆️
samtranslator/metrics/method_decorator.py 100.00% <100.00%> (ø)
samtranslator/metrics/metrics.py 100.00% <100.00%> (ø)
... and 25 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 77deed7...8ee3d81. Read the comment docs.

@mgrandis mgrandis marked this pull request as draft July 30, 2021 16:14
Copy link
Contributor

@CoshUS CoshUS left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Two small changes.

samtranslator/model/eventsources/pull.py Outdated Show resolved Hide resolved
samtranslator/model/eventsources/pull.py Outdated Show resolved Hide resolved
Copy link
Contributor

@CoshUS CoshUS left a comment

Choose a reason for hiding this comment

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

LGTM!

@mgrandis mgrandis marked this pull request as ready for review September 30, 2021 21:09
Copy link
Contributor

@mgrandis mgrandis left a comment

Choose a reason for hiding this comment

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

Good to go, just a small non blocking change.

samtranslator/model/eventsources/pull.py Outdated Show resolved Hide resolved
@jfuss jfuss merged commit 3d3da17 into aws:develop Oct 8, 2021
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.

5 participants