-
Notifications
You must be signed in to change notification settings - Fork 0
Remove automatic STACKONE_ACCOUNT_ID environment variable loading #23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove automatic STACKONE_ACCOUNT_ID environment variable loading #23
Conversation
Remove automatic loading of STACKONE_ACCOUNT_ID from environment variables in StackOneToolSet initialization to provide more explicit control over account ID configuration. The account_id parameter now requires explicit provision if needed, improving clarity and preventing unexpected behavior from environment variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR removes automatic environment variable loading for the STACKONE_ACCOUNT_ID in the StackOneToolSet initialization. The change requires explicit provision of the account_id parameter when needed, improving code clarity and preventing unexpected behavior from environment variables.
Key Changes
- Removed automatic fallback to
os.getenv("STACKONE_ACCOUNT_ID")in StackOneToolSet.init() - Updated docstring and documentation to clarify that account_id requires explicit provision
- Maintained the API key's environment variable fallback for authentication purposes
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| stackone_ai/toolset.py | Removed automatic STACKONE_ACCOUNT_ID environment variable loading and updated docstring |
| CLAUDE.md | Updated documentation to clarify that account_id requires explicit provision |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| toolset = StackOneToolSet( | ||
| api_key="your-api-key", # or STACKONE_API_KEY env var | ||
| account_id="optional-id" # or STACKONE_ACCOUNT_ID env var | ||
| account_id="optional-id" # explicit account ID required |
Copilot
AI
Aug 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment 'explicit account ID required' is contradictory since the parameter is still optional (as indicated by 'optional-id'). Consider changing to 'explicit account ID if needed' or similar to clarify that while the parameter is optional, when provided it must be explicit.
| account_id="optional-id" # explicit account ID required | |
| account_id="optional-id" # explicit account ID if needed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ryoppippi good flag here from copilot, is this optional (as the value seems to indicate) or is this required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no we can set it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 2 files. Review in cubic
glebedel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| toolset = StackOneToolSet( | ||
| api_key="your-api-key", # or STACKONE_API_KEY env var | ||
| account_id="optional-id" # or STACKONE_ACCOUNT_ID env var | ||
| account_id="optional-id" # explicit account ID required |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ryoppippi good flag here from copilot, is this optional (as the value seems to indicate) or is this required?
Summary
This PR removes the automatic loading of the STACKONE_ACCOUNT_ID environment variable from the StackOneToolSet initialization. This change provides more explicit control over account ID configuration.
Changes Made
Rationale
Testing
Test plan
Summary by cubic
Removed automatic loading of STACKONE_ACCOUNT_ID in StackOneToolSet; account_id is now used only when passed explicitly. This makes configuration predictable while keeping STACKONE_API_KEY env fallback; docs updated.