[Bambenek] create bambenek connector #3280
Conversation
romain-filigran
added
community
richard-julien
force-pushed
the
master
branch
2 times, most recently
from
9818df2 to
0f4b30d
Compare
helene-nguyen
changed the title
There was a problem hiding this comment.
Thank you for this contribution.
The code is clear and appears to be mostly robust.
I'd like you to correct the default values for config_variables and update the documentation slightly. Otherwise, I've made some minor suggestions for code improvements that are not mandatory but could make your connector more robust.
I’m unable to test the full functionality of your connector end-to-end since I don’t have the necessary credential
I'll be happy to merge your work once you've updated the code.
Feel free to reach out if you have any questions or comments.
external-import/bambenek/src/bambenek_connector/config_variables.py
Outdated
Show resolved
Hide resolved
| # validate collection configured | ||
| for collection in self.config.bambenek_collections: | ||
| if collection not in SUPPORTED_COLLECTIONS: | ||
| self.helper.connector_logger.error( | ||
| f"Unsupported configured: {collection}" | ||
| ) | ||
| self.config.bambenek_collections.remove(collection) |
There was a problem hiding this comment.
[TIP]
This check could be done in config_variables.py to clearly separate module scopes and responsibilities
There was a problem hiding this comment.
I've moved this functionality over
| self.helper.connector_logger.info( | ||
| "[CONNECTOR] Connector has never run..." | ||
| ) | ||
| last_run = None |
There was a problem hiding this comment.
[CLEANING]
last_run var does not seem to be used
SamuelHassine
force-pushed
the
master
branch
2 times, most recently
from
b513b96 to
11a05bf
Compare
|
@flavienSindou the formatting check is failing on an unrelated file 'external-import/crowdstrike/src/crowdstrike_feeds_connector/indicator/builder.py' |
There was a problem hiding this comment.
Thanks for all the changes you made, @larryfinch !
To resolve the issue with the CrowdStrike file, you'll need to rebase your branch with the latest changes from the master branch.
We recently updated the isort and black requirements, which led to some refactoring between the time you started your branch and now (see issue #3338 for details).
Once that's done, I'll be happy to merge your PR and include your new connector in our codebase.
|
@flavienSindou I pulled in the updates and everything is passing! |
Proposed changes
Related issues
Checklist
Further comments
I've tested that all the individual components work as expected. Testing everything as a whole was not feasible due to my organization using private certs as the requests library just returns certificate errors. I did confirm that it loads configs and initializes as expected when run with docker compose
Please let me know if there's any questions or issues