mp: Added no-ssl verification in dev-env deploy #367
Conversation
Summary of ChangesHello @lemanczykp, 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 introduces a new Highlights
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}
There was a problem hiding this comment.
Code Review
This pull request introduces a --no-ssl flag to the dev-env login and dev-env push commands, allowing users to disable SSL certificate verification. This is a useful feature for local development environments. The implementation correctly passes the flag down to the BackendAPI to control the requests session's verification behavior.
My review includes a few suggestions to improve the code:
- Refactoring the
BackendAPIinstantiation in bothloginandpushcommands to reduce code duplication. - Suppressing the
InsecureRequestWarningthatrequestsissues when SSL verification is disabled, which will improve the user experience as mentioned in the PR description.
Overall, the changes are well-implemented and address the intended goal.
| if api_key is not None: | ||
| backend_api = api.BackendAPI(api_root=params.api_root, api_key=params.api_key) | ||
| backend_api = api.BackendAPI(api_root=params.api_root, api_key=params.api_key, verify_ssl=not no_ssl) | ||
| else: | ||
| backend_api = api.BackendAPI( | ||
| api_root=params.api_root, username=params.username, password=params.password | ||
| api_root=params.api_root, username=params.username, password=params.password, verify_ssl=not no_ssl | ||
| ) |
There was a problem hiding this comment.
To improve maintainability and reduce code duplication, you can refactor the instantiation of BackendAPI. By building a dictionary of keyword arguments, you can avoid repeating the common parameters.
| if api_key is not None: | |
| backend_api = api.BackendAPI(api_root=params.api_root, api_key=params.api_key) | |
| backend_api = api.BackendAPI(api_root=params.api_root, api_key=params.api_key, verify_ssl=not no_ssl) | |
| else: | |
| backend_api = api.BackendAPI( | |
| api_root=params.api_root, username=params.username, password=params.password | |
| api_root=params.api_root, username=params.username, password=params.password, verify_ssl=not no_ssl | |
| ) | |
| kwargs = { | |
| "api_root": params.api_root, | |
| "verify_ssl": not no_ssl, | |
| } | |
| if api_key is not None: | |
| kwargs["api_key"] = params.api_key | |
| else: | |
| kwargs["username"] = params.username | |
| kwargs["password"] = params.password | |
| backend_api = api.BackendAPI(**kwargs) |
| if config.get("api_key"): | ||
| backend_api = api.BackendAPI(api_root=config["api_root"], api_key=config["api_key"]) | ||
| backend_api = api.BackendAPI( | ||
| api_root=config["api_root"], api_key=config["api_key"], verify_ssl=not no_ssl | ||
| ) | ||
| else: | ||
| backend_api = api.BackendAPI( | ||
| api_root=config["api_root"], | ||
| username=config["username"], | ||
| password=config["password"], | ||
| verify_ssl=not no_ssl, | ||
| ) |
There was a problem hiding this comment.
Similar to the login function, you can refactor this section to reduce code duplication and improve readability by preparing the arguments for BackendAPI in a dictionary.
kwargs = {
"api_root": config["api_root"],
"verify_ssl": not no_ssl,
}
if config.get("api_key"):
kwargs["api_key"] = config["api_key"]
else:
kwargs["username"] = config["username"]
kwargs["password"] = config["password"]
backend_api = api.BackendAPI(**kwargs)
packages/mp/src/mp/dev_env/api.py
Outdated
| self.password = password | ||
| self.api_key = api_key | ||
| self.session = requests.Session() | ||
| self.session.verify = verify_ssl |
There was a problem hiding this comment.
The pull request description mentions that new warnings are thrown when using --no-ssl. These are likely InsecureRequestWarning from the requests library. To improve the user experience for local development, you can suppress these warnings when SSL verification is disabled. This avoids cluttering the output for users who are intentionally disabling verification.
self.session.verify = verify_ssl
if not verify_ssl:
import urllib3
urllib3.disable_warnings(urllib3.exceptions.InsecureRequestWarning)
|
Hey, do you want to fix the lint issues ? |
|
@doriaviram Hi, this feature is mostly for ease of local development and was not requested. The lint issues require a bit of rework and retest for which I recently did not have "spare" time. Will try to wrap it during this or next week. |
3 participants
mp tool: Optional disabling ssl verification for dev-env push
Description
Disables ssl when pushing the integration to a development env. This is usefull for localhost deployments.
Example usage:
When using this param the tools throws new warnings
Checklist:
Please ensure you have completed the following items before submitting your PR.
This helps us review your contribution faster and more efficiently.
General Checks:
Open-Source Specific Checks:
For Google Team Members and Reviewers Only: