Skip to content

feat!: remove support for old profile name #5

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.

Sign up for GitHub

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

Open
n-tucker wants to merge 4 commits into
base: main
Choose a base branch
from
Open

feat!: remove support for old profile name #5

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ab1b05e
feat!: remove old profile support when setting chinmina path
n-tucker Apr 24, 2025
68359b8
feat!: enforce new profile prefixes when setting config
n-tucker Apr 24, 2025
5d9e0e1
test: default profile is used when unspecified
n-tucker Apr 24, 2025
1006693
test: invalid profiles are ignored, without exiting early
n-tucker Apr 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions credential-helper/buildkite-connector-credential-helper
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,7 @@ fi
# OIDC JWT from the agent. The output of this request is in the expected format,
# so is sent to stdout to be read by git.
TIMEFORMAT='[token vendor = %2Rs]'
# support both the old and new default profile name
# TODO: tidy up this conditional once chinmina profile support is rolled out
if [[ ${profile} == "default" || ${profile} == "repo:default" ]]; then
if [[ ${profile} == "repo:default" ]]; then
path="git-credentials"
else
path="organization/git-credentials/${profile}"
Expand Down
10 changes: 4 additions & 6 deletions hooks/environment
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,12 +92,10 @@ git_config_add "credential.https://github.com.usehttppath" "true"

# iterate over all provided profiles and configure a credential helper for each one
for profile in "${profiles[@]}"; do
# support both the old and new default profile name
# TODO: uncomment the below code once chinmina profile support is rolled out
# if [[ "${profile}" != org:* && "${profile}" != repo:* ]]; then
# echo "~~~ :error: Invalid profile: ${profile}. Must start with 'org:' or 'repo:'"
# exit 1
# fi
if [[ "${profile}" != org:* && "${profile}" != repo:* ]]; then
echo "~~~ :warning: Invalid profile: ${profile}. Must start with 'org:' or 'repo:'. Skipping configuration for this profile."
continue
fi
git_config_add "credential.https://github.com.helper" "${plugin_root}/credential-helper/buildkite-connector-credential-helper ${chinmina_url} ${audience} ${profile}"
done

26 changes: 8 additions & 18 deletions tests/environment.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ run_environment() {
assert_line --partial "Missing required parameter chinmina-url"
}

@test "Adds config for default audience" {
@test "Adds config for default audience and profile" {
export BUILDKITE_PLUGIN_CHINMINA_GIT_CREDENTIALS_CHINMINA_URL=http://test-location

run_environment "${PWD}/hooks/environment"
Expand All @@ -55,7 +55,7 @@ run_environment() {
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 chinmina:default"
assert_line --regexp "GIT_CONFIG_VALUE_1=/.*/credential-helper/buildkite-connector-credential-helper http://test-location chinmina:default repo:default"
}

@test "Adds config for non-default audience" {
Expand All @@ -72,20 +72,6 @@ run_environment() {
assert_line --regexp "GIT_CONFIG_VALUE_1=/.*/credential-helper/buildkite-connector-credential-helper http://test-location test-audience"
}

@test "Adds config for default profile" {
export BUILDKITE_PLUGIN_CHINMINA_GIT_CREDENTIALS_CHINMINA_URL=http://test-location
export BUILDKITE_PLUGIN_CHINMINA_GIT_CREDENTIALS_AUDIENCE=test-audience

run_environment "${PWD}/hooks/environment"

assert_success
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 repo:default"
}

@test "Adds config for non-default profiles" {
export BUILDKITE_PLUGIN_CHINMINA_GIT_CREDENTIALS_CHINMINA_URL=http://test-location
export BUILDKITE_PLUGIN_CHINMINA_GIT_CREDENTIALS_AUDIENCE=test-audience
Expand All @@ -104,18 +90,22 @@ run_environment() {
assert_line --regexp "GIT_CONFIG_VALUE_2=/.*/credential-helper/buildkite-connector-credential-helper http://test-location test-audience repo:another-test-profile"
}

@test "Backwards compatible with old name" {
@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"
}
Comment on lines +93 to 109
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 24, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

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-audience

All 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.


@test "Adds to existing configuration if present" {
Expand Down