fix(vercel): Correctly fetch all environment variables for Vercel integration#118622
Open
sentry[bot] wants to merge 2 commits into
Open
fix(vercel): Correctly fetch all environment variables for Vercel integration#118622sentry[bot] wants to merge 2 commits into
sentry[bot] wants to merge 2 commits into
Conversation
Comment on lines
94
to
98
|
|
||
| def get_env_vars(self, vercel_project_id): | ||
| return self.get(self.GET_ENV_VAR_URL % vercel_project_id) | ||
| results = self.get_from_pagination(self.GET_ENV_VAR_URL % vercel_project_id, "envs") | ||
| return {"envs": results} | ||
|
|
Contributor
Author
There was a problem hiding this comment.
Bug: The get_from_pagination function unconditionally accesses response["pagination"]["next"], but the Vercel API omits this key for single-page responses, causing a KeyError.
Severity: CRITICAL
Suggested Fix
In get_from_pagination, safely access the pagination data to handle cases where the key is not present. Use response.get("pagination", {}) to provide a default empty dictionary, and then use .get("next") on the result to safely retrieve the cursor. This will return None if the pagination key or the next key is missing, preventing the KeyError.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: src/sentry/integrations/vercel/client.py#L94-L98
Potential issue: The `get_env_vars` function now utilizes `get_from_pagination` to fetch
environment variables from the Vercel API. This helper function unconditionally accesses
`response["pagination"]["next"]`, which will raise a `KeyError`. According to Vercel's
API documentation, the `pagination` key is optional and is omitted for responses that
fit on a single page, such as for projects with fewer than 100 environment variables.
This is a common scenario, meaning the integration will fail for any Vercel project with
a small number of environment variables, preventing environment variable
synchronization.
Did we get this right? 👍 / 👎 to inform future reviews.
Member
|
@sentry fix the CI bugs |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The Vercel integration's
get_env_varsmethod was not correctly handling pagination for environment variables. This meant that if an environment variable was not present on the first page of the Vercel API response, the integration would fail to find it, leading to anIntegrationErrorwhen attempting to update it.This change updates
get_env_varsinsrc/sentry/integrations/vercel/client.pyto useself.get_from_pagination. This ensures that all pages of environment variables are fetched, allowing the integration to find and update existing variables regardless of their position in the paginated response. The results are then wrapped in a{"envs": results}dictionary to maintain the expected return signature.Additionally, the mocked Vercel API response in
tests/sentry/integrations/vercel/test_integration.pyhas been updated to include"pagination": {"next": None}to correctly simulate the paginated API behavior and prevent test failures.Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.
Fixes SENTRY-5Q81
Comment
@sentry <feedback>on this PR to have Autofix iterate on the changes.