Respect SKIP_SETTING_JIRA_FIELDS environment variable #375
Conversation
![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 adds support for the SKIP_SETTING_JIRA_FIELDS environment variable to prevent updates to Jira fields, similar to the existing DRY_RUN variable. The implementation is straightforward. My review includes a suggestion to refactor the condition for checking environment variables to improve readability and reduce duplication. I've also noted that a test case for the new functionality is missing and should be added to ensure the change is properly verified.
Signed-off-by: Nikola Forró <[email protected]>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a SKIP_SETTING_JIRA_FIELDS environment variable to bypass updating Jira fields, similar to the existing DRY_RUN functionality. The implementation is straightforward. My review includes a suggestion to make the environment variable check more robust and a recommendation to add test coverage for the new logic branch to ensure correctness.
| flexmock(os).should_receive("getenv").with_args("JIRA_URL").and_return("http://jira") | ||
| flexmock(os).should_receive("getenv").with_args("JIRA_TOKEN").and_return("12345") | ||
| flexmock(os).should_receive("getenv").with_args("DRY_RUN", "False").and_return("false") | ||
| flexmock(os).should_receive("getenv").with_args("SKIP_SETTING_JIRA_FIELDS", "False").and_return("false") |
There was a problem hiding this comment.
While this change correctly mocks the new environment variable for existing tests, the new functionality itself is not tested. A new test case should be added to verify that when SKIP_SETTING_JIRA_FIELDS is set to a truthy value, the set_jira_fields function returns the expected skip message and does not attempt to make any API calls to Jira. Untested code paths can introduce regressions in the future.
| """ | ||
| Updates the specified Jira issue, setting only the fields that are currently empty/unset. | ||
| """ | ||
| if os.getenv("SKIP_SETTING_JIRA_FIELDS", "False").lower() == "true": |
There was a problem hiding this comment.
Checking for boolean environment variables by comparing to just 'true' can be brittle, as users might intuitively use other truthy values like '1' or 'yes'. It's more robust to check against a set of common truthy string values. This improvement could also be applied to the DRY_RUN check that follows.
| if os.getenv("SKIP_SETTING_JIRA_FIELDS", "False").lower() == "true": | |
| if os.getenv("SKIP_SETTING_JIRA_FIELDS", "False").lower() in ("true", "1", "yes"): |
2 participants
No description provided.