feat!: remove support for old profile name#5
Conversation
WalkthroughThe changes focus on refining the handling and validation of profile names in credential management scripts. In the credential helper, the logic for determining the credentials path was simplified by eliminating support for the legacy "default" profile, now only checking for "repo:default". In the environment hook, the script now validates each profile during iteration, skipping invalid profiles with a warning message instead of terminating the script. Corresponding tests were updated to reflect these changes, including renaming, removal, and addition of tests to verify the new validation behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EnvHook as Environment Hook
participant CredentialHelper
User->>EnvHook: Run environment setup
EnvHook->>EnvHook: For each profile in profiles
alt Profile starts with "org:" or "repo:"
EnvHook->>CredentialHelper: Configure valid profile
else Invalid profile
EnvHook->>EnvHook: Print warning, skip profile
end
EnvHook-->>User: Environment setup continues
User->>CredentialHelper: Request credentials with profile
alt profile == "repo:default"
CredentialHelper->>CredentialHelper: Set path to "git-credentials"
else
CredentialHelper->>CredentialHelper: Set path to "organization/git-credentials/${profile}"
end
CredentialHelper-->>User: Return credentials
Possibly related PRs
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
now we skip invalid profiles, failing early would cause subsequent credential helpers to never be configured. this limits the blast radius of invalid config
n-tucker
force-pushed
the
nt/remove-old-profile-support
branch
from
09f5795 to
68359b8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
hooks/environment(1 hunks)tests/environment.bats(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- hooks/environment
🔇 Additional comments (2)
tests/environment.bats (2)
48-59: Test name updated to reflect new profile naming requirement.The test name has been updated to explicitly include the profile verification, which aligns with the PR objective of removing support for old profile names. The assertion now correctly verifies the presence of the "repo:default" profile argument, enforcing the new profile naming convention that requires explicit prefixes.
102-103: Warning message handling aligns with PR objectives.The warning message handling correctly implements the PR's intention to validate profiles individually and continue execution even if some profiles are invalid. This is a more graceful approach than terminating the script entirely when encountering invalid profiles.
| @test "Ignores profiles without specified prefixes" { | ||
| export BUILDKITE_PLUGIN_GITHUB_APP_AUTH_VENDOR_URL=http://test-location | ||
| export BUILDKITE_PLUGIN_GITHUB_APP_AUTH_AUDIENCE=test-audience | ||
| export BUILDKITE_PLUGIN_CHINMINA_GIT_CREDENTIALS_PROFILES_0="invalid-profile-name" | ||
| export BUILDKITE_PLUGIN_CHINMINA_GIT_CREDENTIALS_PROFILES_1="org:read-packages" | ||
|
|
||
| run_environment "${PWD}/hooks/environment" | ||
|
|
||
| assert_success | ||
| assert_line --partial ":warning: Invalid profile" | ||
|
|
||
| assert_line "GIT_CONFIG_COUNT=2" | ||
| assert_line "GIT_CONFIG_KEY_0=credential.https://github.com.usehttppath" | ||
| assert_line "GIT_CONFIG_VALUE_0=true" | ||
| assert_line "GIT_CONFIG_KEY_1=credential.https://github.com.helper" | ||
| assert_line --regexp "GIT_CONFIG_VALUE_1=/.*/credential-helper/buildkite-connector-credential-helper http://test-location test-audience" | ||
| assert_line --regexp "GIT_CONFIG_VALUE_1=/.*/credential-helper/buildkite-connector-credential-helper http://test-location test-audience org:read-packages" | ||
| } |
There was a problem hiding this comment.
Environment variable inconsistency in test setup.
The test correctly validates the new behavior of ignoring invalid profiles (without proper prefixes) while continuing with valid ones. However, the URL and audience environment variables use an inconsistent naming pattern:
- export BUILDKITE_PLUGIN_GITHUB_APP_AUTH_VENDOR_URL=http://test-location
- export BUILDKITE_PLUGIN_GITHUB_APP_AUTH_AUDIENCE=test-audience
+ export BUILDKITE_PLUGIN_CHINMINA_GIT_CREDENTIALS_CHINMINA_URL=http://test-location
+ export BUILDKITE_PLUGIN_CHINMINA_GIT_CREDENTIALS_AUDIENCE=test-audienceAll other tests in this file use the CHINMINA_GIT_CREDENTIALS prefix for environment variables, not GITHUB_APP_AUTH.
Committable suggestion skipped: line range outside the PR's diff.
1 participant
Warning
This is a breaking change!
Purpose
Context
Summary by CodeRabbit
Refactor
Bug Fixes