Add envcommon to repo #439
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
❌ Marketplace Validation Failed Click to view the full reportValidation Report🧩 IntegrationsPre-Build Stagephish_tank
cybersixgill_darkfeed_enrichment
insights
cybersixgill_dve_feed
image_utilities
aws_ec2
microsoft_graph_security_tools
torq
grey_noise
spell_checker
full_contact
data_dog
be_secure
cylusone
google_drive
zoom
vectra_qux
docker_hub
clarotyxdome
enrichment
google_safe_browsing
eclectic_iq
jamf
connectors
lists
darktrace
abuse_ipdb
nucleon_cyber
asana
marketo
recorded_future_intelligence
logzio
anyrun_sandbox
tools
bandura_cyber
thinkst_canary
cybersixgill_actionable_alerts
webhook
xm_cyber
arcanna_ai
chronicle_support_tools
perimeter_x
vorlon
google_docs
vectra_rux
whois_xml_api
philips_hue
lacework
infoblox_nios
send_grid
bitdefender_gravity_zone
hibob
group_ib_ti
git_sync
spycloud
luminar_iocs_and_leaked_credentials
team_cymru_scout
flashpoint
air_table
pager_duty
imgbb
telegram
cybersixgill_dve_enrichment
pulsedive
superna_zero_trust
sample_integration
functions
template_engine
netenrich_connect
stairwell
cybersixgill_darkfeed
workflow_tools
vanilla_forums
doppel_vision
azure_devops
duo
anyrun_ti_lookup
ipqs_fraud_and_risk_scoring
anyrun_ti_feeds
google_sheets
wiz
country_flags
orca_security
file_utilities
infoblox_threat_defense_with_ddi
email_utilities
|
Contributor
Summary of ChangesHello @TalShafir1, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request integrates a new Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a new shared package envcommon for handling environment logic and updates numerous pyproject.toml files to use a new path for the environmentcommon wheel.
The introduction of the envcommon package is a good step towards centralizing common logic. However, the new code in EnvironmentHandle.py has several issues that need to be addressed:
- There are violations of the repository's style guide, particularly regarding path handling (
os.path.joininstead ofpathlib) and the use ofjson.loads. - The docstrings do not follow the required Google Style format.
- There is a critical bug in a logging call (
self.logger.LOGGER.error) that will cause a runtime error. - The regex matching logic could be made more robust to correctly handle capturing groups.
Please see the detailed comments for specific suggestions on how to resolve these issues.
| default_environment=siemplify.context.connector_info.environment, | ||
| ) | ||
|
|
||
| map_file_path: str = os.path.join(siemplify.run_folder, map_file) |
Contributor
There was a problem hiding this comment.
The repository style guide (lines 35-37) mandates the use of pathlib.Path for all filesystem operations and explicitly forbids os.path.join. Please use pathlib for path manipulation.
For a more robust implementation, consider refactoring validate_map_file_exists and EnvironmentHandleForFileSystem to work with pathlib.Path objects directly, which would avoid converting back and forth between strings and Path objects.
map_file_path: str = str(pathlib.Path(siemplify.run_folder) / map_file)References
- The style guide requires using
pathlib.Pathfor all file system operations and explicitly disallows usingos.path.join. (link)
| mappings = {} | ||
|
|
||
| if not isinstance(mappings, dict): | ||
| self.logger.LOGGER.error( |
Contributor
There was a problem hiding this comment.
There seems to be a typo here. It should be self.logger.error instead of self.logger.LOGGER.error. This will cause a runtime AttributeError and prevent logging of this important error condition.
self.logger.error(| """ | ||
| try: | ||
| with pathlib.Path(self.map_file_path).open("r+", encoding="utf-8") as map_file: | ||
| mappings = json.loads(map_file.read()) |
Contributor
There was a problem hiding this comment.
The repository style guide (lines 45-46) prohibits the use of json.loads(). While json.loads() is generally considered safe, the style guide is strict on this point.
The guide suggests using yaml.safe_load(), but that is for YAML files and will not work for JSON. Given the conflicting and likely incorrect guidance, I am flagging this as a high severity issue based on the explicit rule. You should probably consult with the repo maintainers to clarify or correct the style guide.
References
- The style guide prohibits using
json.loads()for safety reasons, suggestingyaml.safe_load()as an alternative. (link)
Comment on lines
+63
to
+69
| """Get environment common | ||
| :param siemplify: {siemplify} Siemplify object | ||
| :param environment_field_name: {string} The environment field name | ||
| :param environment_regex_pattern: {string} The environment regex pattern | ||
| :param map_file: {string} The map file | ||
| :return: {EnvironmentHandle} | ||
| """ |
Contributor
There was a problem hiding this comment.
The repository style guide (line 88) requires Google Style Docstrings. This docstring uses a different format (:param, :return:). Please update it to follow the Google style. Also, as per line 91, do not include types in the Args or Returns sections.
"""Get environment common.
Args:
siemplify: Siemplify object.
environment_field_name: The environment field name.
environment_regex_pattern: The environment regex pattern.
map_file: The map file.
Returns:
An instance of an EnvironmentHandle implementation.
"""References
- All modules, classes, and functions must use Google Style Docstrings with triple double quotes. Types should not be repeated in the Args or Returns sections as they are inferred from type hints. (link)
|
|
||
| if match: | ||
| # Get the first matching value to match the pattern | ||
| environment = match.group() |
Contributor
There was a problem hiding this comment.
Using match.group() returns the entire matched string. If your regex pattern uses a capturing group to extract a specific part of the environment string, this will not behave as expected. For example, if the environment is domain\\user and the regex is domain\\(.*), match.group() returns domain\\user, but match.group(1) would correctly return user.
To make this more robust, I suggest prioritizing the first captured group if it exists, and falling back to the full match otherwise.
environment = match.group(1) if match.groups() else match.group(0)|
|
||
| if match: | ||
| # Get the first matching value to match the pattern | ||
| environment = match.group() |
Contributor
There was a problem hiding this comment.
Using match.group() returns the entire matched string. If your regex pattern uses a capturing group to extract a specific part of the environment string, this will not behave as expected. For example, if the environment is domain\\user and the regex is domain\\(.*), match.group() returns domain\\user, but match.group(1) would correctly return user.
To make this more robust, I suggest prioritizing the first captured group if it exists, and falling back to the full match otherwise.
environment = match.group(1) if match.groups() else match.group(0)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.