Add a function URL enabling public access to Lambda function #209
Conversation
…ons and improve error handling
…d improve error handling
…ove response handling
aaarendt
requested review from
jomey,
micah-prime and
micahjohnson150
as code owners
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #209 +/- ##
==========================================
- Coverage 60.25% 59.44% -0.81%
==========================================
Files 23 23
Lines 1195 1218 +23
==========================================
+ Hits 720 724 +4
- Misses 475 494 +19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jomey
left a comment
jomey
left a comment
There was a problem hiding this comment.
Mostly minor comments and curious thoughts
| If None, uses SNOWEX_LAMBDA_URL environment variable | ||
| or default production URL. | ||
| No AWS credentials required - uses public HTTP endpoint. |
There was a problem hiding this comment.
Minor: Would move this description above the Args to consolidate the method description.
| function_url or | ||
| os.environ.get('SNOWEX_LAMBDA_URL') or | ||
| self.DEFAULT_FUNCTION_URL | ||
| ) |
There was a problem hiding this comment.
Feels like this should be documented somewhere that there are three options to set the URL
|
|
||
| # Validate URL | ||
| if not self.function_url or self.function_url == 'PASTE_YOUR_FUNCTION_URL_HERE': | ||
| raise ValueError( |
There was a problem hiding this comment.
Curious where does this comparison value come from?
| ) | ||
| adapter = HTTPAdapter(max_retries=retry_strategy) | ||
| self.session.mount("https://", adapter) | ||
| self.session.mount("http://", adapter) |
There was a problem hiding this comment.
Curious why the endpoint is mounted for both http and https although the URL is configured as only https. Can we just not support http?
| region: AWS region where the Lambda function is deployed | ||
| function_name: Name of the deployed Lambda function | ||
| function_url: Lambda Function URL (https://....lambda-url.us-west-2.on.aws/) | ||
| If None, uses SNOWEX_LAMBDA_URL environment variable |
There was a problem hiding this comment.
This example URL shows a trailing / although it is later removed in the method logic block. Think this example should not show one to start with a proper formed URL
| self.function_url, | ||
| json=payload, | ||
| headers={'Content-Type': 'application/json'}, | ||
| timeout=30 # 30 second timeout |
There was a problem hiding this comment.
Minor: I like to have numbers like this as class constants. Then you can omit the comment when the variable is descriptive.
| f"1. The Function URL is correct\n" | ||
| f"2. You have internet connectivity\n" | ||
| f"3. The Lambda function is deployed and active\n\n" | ||
| f"Connection error: {str(e)}" |
There was a problem hiding this comment.
Putting myself into the shoes of an end-user - Is this a helpful message? For instance, how much power does one have over the construction of the URL? Can one influence whether this function is properly deployed?
| "SQLAlchemy <3.0", | ||
| "boto3 <2.0", | ||
| "requests >=2.31.0", | ||
| "urllib3 >=2.0", |
There was a problem hiding this comment.
Do these also need to be restricted to <3 in case a major version comes out with breaking changes?
| There are two ways to access SnowEx data through this library: | ||
|
|
||
| 1. **Direct Database Access** (requires database credentials) | ||
| 2. **Lambda Client** (requires AWS credentials for serverless access) |
There was a problem hiding this comment.
Should this now say “no credentials” with the function URL?
| ) | ||
| print(df.head()) | ||
| Lambda Client (Serverless Access) |
There was a problem hiding this comment.
Should we help the user out here and indicate this as the preferred one?
Could be just added to the parenthesis
This PR adds a function URL enabling a publicly accessible dedicated HTTPS endpoint. Prior to this users required having access to AWS credentials which we don't want to distribute.