Skip to content

Conversation

@hovu96
Copy link
Contributor

@hovu96 hovu96 commented Oct 29, 2025

Summary

Fixes the UX issue where workato init silently used WORKATO_API_TOKEN and WORKATO_HOST environment variables without informing the user, causing confusion about which credentials were being used.

Changes

Interactive Mode:

  • Environment variables are now detected before the profile selection menu
  • Clear messages show what was detected
  • User is explicitly asked: "Use environment variables for authentication?"
  • If yes: User only selects profile name (credentials from env vars)
  • If no: Normal profile selection with credential prompts

Non-Interactive Mode:

  • Automatically uses env vars as fallback when CLI flags aren't provided
  • CLI flags always take precedence over env vars

Technical Details

  • Moved env var detection from _create_new_profile() to _setup_profile()
  • Added _select_profile_name_for_env_vars() for profile name selection
  • Added _create_profile_with_env_vars() for creating profiles with env var credentials
  • Simplified _create_new_profile() by removing env var detection logic
  • Updated non-interactive mode validation in init.py

Testing

✅ All 905 tests passing
✅ Basic manual testing completed
⚠️ Needs thorough manual testing - see DEVP-422 for test scenarios

Related Issues

  • Fixes DEVP-436
  • Related to DEVP-422

🤖 Generated with Claude Code

Refactored environment variable detection to happen before profile selection,
providing clear user feedback and explicit confirmation before using env vars.

Changes:
- Move env var detection from _create_new_profile() to _setup_profile()
- Detect WORKATO_API_TOKEN and WORKATO_HOST before showing profile menu
- Ask user "Use environment variables for authentication?" with clear messaging
- Add _select_profile_name_for_env_vars() to select profile name without credentials
- Add _create_profile_with_env_vars() to create profile using env var credentials
- Simplify _create_new_profile() by removing env var detection logic
- Update non-interactive mode in init.py to use env vars automatically
- Remove 5 obsolete tests that tested old env var behavior

This fixes the confusing UX where selecting an existing profile would silently
use environment variable credentials without informing the user.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@github-actions
Copy link

github-actions bot commented Oct 29, 2025

Coverage

Coverage Report
FileStmtsMissCoverMissing
__init__.py68494%10–11, 69–70
cli
   __init__.py52394%49, 51, 60
   containers.py270100% 
cli/commands
   __init__.py00100% 
   api_clients.py291996%27, 302, 448–449, 483, 493, 584, 615, 623
   api_collections.py257398%28, 183, 347
   assets.py47295%55–56
   connections.py526599%591, 593, 599, 637, 988
   data_tables.py165596%31, 253, 267, 321–322
   guide.py166199%106
   init.py1070100% 
   profiles.py2030100% 
   properties.py97198%21
   pull.py172298%193–194
   workspace.py39294%61, 71
cli/commands/connectors
   __init__.py00100% 
   command.py90297%110, 159
   connector_manager.py203498%176, 292, 300–301
cli/commands/projects
   __init__.py00100% 
   command.py2721096%359–362, 373, 439–441, 491, 495
   project_manager.py166795%48, 66, 263–264, 276, 317, 325
cli/commands/push
   __init__.py00100% 
   command.py133496%109, 112, 230, 308
cli/commands/recipes
   __init__.py00100% 
   command.py427997%117, 133–134, 272–275, 403, 709
   validator.py7062097%174, 883, 1136, 1223, 1246, 1279, 1281–1282, 1359–1361, 1457–1458, 1517–1518, 1707–1708, 1736–1738
cli/utils
   __init__.py30100% 
   exception_handler.py3042591%134–135, 140–141, 143–144, 170–171, 177, 220, 267, 294, 321, 378, 413, 472, 474–475, 480–481, 483–487
   gitignore.py140100% 
   ignore_patterns.py230100% 
   spinner.py430100% 
   version_checker.py135695%24, 26, 33–34, 72, 102
cli/utils/config
   __init__.py50100% 
   manager.py5483793%126, 137, 148–150, 153, 156, 168, 218–219, 365, 383–384, 388, 391–395, 412–413, 434–435, 446–447, 460, 500, 581–582, 621, 835, 978–979, 993–994, 1055, 1114
   models.py330100% 
   profiles.py3091096%93, 189–190, 193, 228–230, 255–257
   workspace.py680100% 
TOTAL569917197% 

@hovu96 hovu96 marked this pull request as draft October 29, 2025 11:52
@hovu96 hovu96 requested review from cmworkato and oalami October 29, 2025 11:56
@oalami oalami requested a review from Copilot October 30, 2025 21:34
Copy link
Contributor

Copilot AI left a 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 adds support for automatically detecting and using WORKATO_API_TOKEN and WORKATO_HOST environment variables during CLI initialization, allowing non-interactive mode to work without explicit CLI flags when these environment variables are set.

Key changes:

  • Environment variables are checked and used as fallback values when CLI flags are not provided
  • Region detection is enhanced to automatically determine region from WORKATO_HOST URL
  • Interactive mode prompts users to confirm usage of detected environment variables

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/workato_platform_cli/cli/commands/init.py Added environment variable detection in non-interactive mode and updated error messages to reference env vars
src/workato_platform_cli/cli/utils/config/manager.py Implemented env var detection, region matching from URLs, and new profile creation flow for env vars
tests/unit/config/test_manager.py Added test coverage for non-interactive mode with environment variables and region detection
Comments suppressed due to low confidence (1)

src/workato_platform_cli/cli/utils/config/manager.py:146

  • This statement is unreachable.
                api_url = region_info.url

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
hovu96 and others added 4 commits October 31, 2025 10:52
Remove unreachable dead code that checked api_url after it was already used, and add explicit warning when overwriting existing profiles with environment variable credentials to prevent accidental data loss.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
Update non-interactive mode validation to accept either --region or --api-url
(along with --api-token). The region detection logic already handles extracting
region from api_url, so the validation should reflect this capability.

This prevents confusing error messages when users provide --api-url without
--region, since the code can auto-detect region from the URL.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
Fix duplicate self parameter in test_setup_profile_existing_create_new_success
that was accidentally introduced during refactoring.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@hovu96 hovu96 marked this pull request as ready for review October 31, 2025 11:18
@hovu96 hovu96 requested a review from oalami October 31, 2025 11:18
oalami
oalami previously approved these changes Oct 31, 2025
Copy link
Collaborator

@oalami oalami left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, did a bunch of manual testing and it working smoothly. @cmworkato please take a pass as well

@cmworkato
Copy link
Collaborator

I'm still seeing issues.

Test 1: When ENV Vars are set in the current shell, workato workspace fails. Running workato init prompts to enter URL and Token. Unsetting ENV Vars has no effect. As far as I can tell, Workato-CLI is ignoring ENV Vars entirely and relying solely on the Key Chain. Is that what we want? My concern is we do not have the ability to perform full testing of Keychain across OSes (Windows/Linux).

Steps to reproduce:

  1. Rotate your API token in Workato
  2. /bin/rm -r ~/.workato
  3. Delete Keychain:workato-platform-cli
  4. Remove ENV vars from ~/.zshrc
  5. export WORKATO_API_HOST and WORKATO_API_TOKEN in the current shell
  6. workato workspace
  7. workato init

Output:

✗ env | grep WORKATO
WORKATO_API_HOST=https://app.preview.workato.com
WORKATO_API_TOKEN=wrkaus-eyJ0eXAiOiJKV--REDACTED--

(workato-platform-cli) ➜ workato-platform-cli git:(bugfix/detect-env-vars-in-init) ✗ workato workspace
❌ ValueError
Could not resolve API credentials. Please check your profile configuration or run 'workato init' to set up authentication.

@cmworkato
Copy link
Collaborator

cmworkato commented Nov 3, 2025 via email

hovu96 and others added 2 commits November 4, 2025 09:57
This is a pure refactoring with no functional changes to user-facing behavior.

Changes:
- Replaced ~70 lines of duplicated region selection code in both
  _create_profile_with_env_vars() and _create_new_profile() methods
- Both methods now call the existing ProfileManager.select_region_interactive()
  helper method instead of duplicating the logic
- Updated test mocks to use the helper method instead of mocking inline code

Benefits:
- Eliminates code duplication (DRY principle)
- Adds missing URL security validation (HTTP only allowed for localhost)
- Adds URL normalization (strips path components)
- Improves maintainability - changes only needed in one place
- Net reduction of 54 lines of code

Testing:
- All 921 tests pass
- Updated 2 existing tests to mock the helper method
- Manual testing confirms all flows work correctly:
  * workato init with both WORKATO_API_TOKEN and WORKATO_HOST
  * workato init with only WORKATO_API_TOKEN (no host)
  * workato init with no environment variables

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
hovu96 and others added 3 commits November 4, 2025 11:35
…e setup

Add 6 new test cases to verify environment variable detection behavior
during profile initialization:
- Both WORKATO_API_TOKEN and WORKATO_HOST detected
- User declining to use detected env vars
- Partial env vars requiring additional input
- Existing profile overwrite confirmation flow
- Cancellation when user declines overwrite

These tests ensure robust handling of environment variables during
the init process and verify proper user confirmation flows.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
Enhanced the user experience when only partial environment variables are
detected (e.g., only WORKATO_API_TOKEN without WORKATO_HOST):

- Explicitly show what's missing with clear messages
- Context-aware confirmation prompts based on detected variables
- Simplified verbose messaging to avoid redundancy
- Removed duplicate prompt headers

Users now get clear expectations about what additional input is needed
without unnecessary repetition or verbose explanations.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
Enhanced transparency in non-interactive mode when environment variables
are used as fallback for CLI flags:

- Regular mode: Shows clear messages when WORKATO_API_TOKEN and/or
  WORKATO_HOST are used from environment
- JSON mode: Maintains valid JSON output without text messages
- CLI flags take precedence: No messages shown when explicit flags provided
- Interactive mode: Completely unchanged (zero impact)

Added comprehensive test coverage:
- test_init_non_interactive_with_env_vars_regular_mode
- test_init_non_interactive_with_env_vars_json_mode
- test_init_non_interactive_cli_flags_take_precedence
- test_init_non_interactive_partial_env_vars_json

All 931 tests pass. Linters and type checks pass.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@hovu96
Copy link
Contributor Author

hovu96 commented Nov 4, 2025

I tested various scenarios with/without the environment variables WORKATO_HOST and WORKATO_API_TOKEN and based on my tests, everything works as expected. Today, I improved the UX messaging when environment variables are detected (making it clearer what's found/missing) and increased test coverage for these scenarios, but no functional changes around credential resolution.

Questions for @cmworkato:

  1. Exact env var names: Are you setting WORKATO_HOST or WORKATO_API_HOST? (The CLI requires WORKATO_HOST, not WORKATO_API_HOST)
  2. Reproduction steps: Can you provide the exact sequence of commands that fail? Your comments are inconsistent:
    • First you mention workato init failing with env vars
    • Then you say it "works" but other commands fail
    • Then you say "other commands run as expected"
  3. Error message: What is the exact error message you're seeing when using the env vars above?

@oalami oalami requested a review from Copilot November 4, 2025 23:41
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (2)

tests/unit/config/test_manager.py:1

  • Removed line appears to be a dictionary key for prompt responses. Verify that the custom URL prompt is no longer needed in this test case or if its removal is intentional. The test still expects custom region handling but may no longer properly mock the URL prompt.
"""Tests for ConfigManager."""

tests/unit/config/test_manager.py:1

  • Removed code was mocking inquirer.prompt for region selection. With the new implementation using select_region_interactive (lines 2005-2012), this removal appears correct. However, verify that the test still properly validates custom region behavior.
"""Tests for ConfigManager."""

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@oalami oalami merged commit 09ca338 into main Nov 4, 2025
11 checks passed
@oalami oalami deleted the bugfix/detect-env-vars-in-init branch November 4, 2025 23:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants