-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(github): add orchestration ID to user-agent in getOctokitOptions #2364
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
Changes from 2 commits
a8ea745
b0917c5
ffeb50b
3643ce2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,114 @@ | ||
| import {getOctokitOptions, getUserAgentWithOrchestrationId} from '../src/utils' | ||
| import {getUserAgentWithOrchestrationId as internalGetUserAgentWithOrchestrationId} from '../src/internal/utils' | ||
|
|
||
| describe('orchestration ID support', () => { | ||
| let originalOrchId: string | undefined | ||
|
|
||
| beforeEach(() => { | ||
| originalOrchId = process.env['ACTIONS_ORCHESTRATION_ID'] | ||
| delete process.env['ACTIONS_ORCHESTRATION_ID'] | ||
| }) | ||
|
|
||
| afterEach(() => { | ||
| if (originalOrchId !== undefined) { | ||
| process.env['ACTIONS_ORCHESTRATION_ID'] = originalOrchId | ||
| } else { | ||
| delete process.env['ACTIONS_ORCHESTRATION_ID'] | ||
| } | ||
| }) | ||
|
|
||
| describe('getUserAgentWithOrchestrationId', () => { | ||
| it('returns undefined when env var is not set and no base user agent', () => { | ||
| expect(getUserAgentWithOrchestrationId()).toBeUndefined() | ||
| }) | ||
|
|
||
| it('returns base user agent unchanged when env var is not set', () => { | ||
| expect(getUserAgentWithOrchestrationId('my-app')).toBe('my-app') | ||
| }) | ||
|
|
||
| it('returns orchestration ID without base when env var is set and no base', () => { | ||
| process.env['ACTIONS_ORCHESTRATION_ID'] = 'abc-123' | ||
| expect(getUserAgentWithOrchestrationId()).toBe( | ||
| 'actions_orchestration_id/abc-123' | ||
| ) | ||
| }) | ||
|
|
||
| it('appends orchestration ID to base user agent', () => { | ||
| process.env['ACTIONS_ORCHESTRATION_ID'] = 'abc-123' | ||
| expect(getUserAgentWithOrchestrationId('my-app')).toBe( | ||
| 'my-app actions_orchestration_id/abc-123' | ||
| ) | ||
| }) | ||
|
|
||
| it('sanitizes special characters in orchestration ID', () => { | ||
| process.env['ACTIONS_ORCHESTRATION_ID'] = 'id with spaces/and$pecial!' | ||
| expect(getUserAgentWithOrchestrationId('my-app')).toBe( | ||
| 'my-app actions_orchestration_id/id_with_spaces_and_pecial_' | ||
| ) | ||
| }) | ||
|
|
||
| it('preserves allowed characters in orchestration ID', () => { | ||
| process.env['ACTIONS_ORCHESTRATION_ID'] = | ||
| 'valid_id-with.allowed_chars.123' | ||
| expect(getUserAgentWithOrchestrationId()).toBe( | ||
| 'actions_orchestration_id/valid_id-with.allowed_chars.123' | ||
| ) | ||
| }) | ||
|
|
||
| it('ignores whitespace-only orchestration ID', () => { | ||
| process.env['ACTIONS_ORCHESTRATION_ID'] = ' ' | ||
| expect(getUserAgentWithOrchestrationId('my-app')).toBe('my-app') | ||
| }) | ||
| }) | ||
|
|
||
| describe('public re-export', () => { | ||
| it('exports getUserAgentWithOrchestrationId from utils (public API)', () => { | ||
| expect(getUserAgentWithOrchestrationId).toBe( | ||
| internalGetUserAgentWithOrchestrationId | ||
| ) | ||
| }) | ||
| }) | ||
|
|
||
| describe('getOctokitOptions', () => { | ||
| it('sets userAgent when ACTIONS_ORCHESTRATION_ID is set', () => { | ||
| process.env['ACTIONS_ORCHESTRATION_ID'] = 'test-orch-id' | ||
| const opts = getOctokitOptions('fake-token') | ||
| expect(opts.userAgent).toBe('actions_orchestration_id/test-orch-id') | ||
| }) | ||
|
|
||
| it('does not set userAgent when ACTIONS_ORCHESTRATION_ID is not set', () => { | ||
| const opts = getOctokitOptions('fake-token') | ||
| expect(opts.userAgent).toBeUndefined() | ||
| }) | ||
|
|
||
| it('preserves and appends to caller-provided userAgent', () => { | ||
| process.env['ACTIONS_ORCHESTRATION_ID'] = 'test-orch-id' | ||
| const opts = getOctokitOptions('fake-token', { | ||
| userAgent: 'custom-agent/1.0' | ||
| }) | ||
| expect(opts.userAgent).toBe( | ||
| 'custom-agent/1.0 actions_orchestration_id/test-orch-id' | ||
| ) | ||
| }) | ||
|
|
||
| it('leaves caller-provided userAgent intact when env var is not set', () => { | ||
| const opts = getOctokitOptions('fake-token', { | ||
| userAgent: 'custom-agent/1.0' | ||
| }) | ||
| expect(opts.userAgent).toBe('custom-agent/1.0') | ||
| }) | ||
|
|
||
| it('does not mutate the original options object', () => { | ||
| process.env['ACTIONS_ORCHESTRATION_ID'] = 'test-orch-id' | ||
| const original = {userAgent: 'original/1.0'} | ||
| getOctokitOptions('fake-token', original) | ||
| expect(original.userAgent).toBe('original/1.0') | ||
| }) | ||
|
|
||
| it('sanitizes special characters through getOctokitOptions', () => { | ||
| process.env['ACTIONS_ORCHESTRATION_ID'] = 'bad chars here!' | ||
| const opts = getOctokitOptions('fake-token') | ||
| expect(opts.userAgent).toBe('actions_orchestration_id/bad_chars_here_') | ||
| }) | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,3 +42,15 @@ export function getProxyFetch(destinationUrl): typeof fetch { | |
| export function getApiBaseUrl(): string { | ||
| return process.env['GITHUB_API_URL'] || 'https://api.github.com' | ||
| } | ||
|
|
||
| export function getUserAgentWithOrchestrationId( | ||
| baseUserAgent?: string | ||
| ): string | undefined { | ||
| const orchId = process.env['ACTIONS_ORCHESTRATION_ID']?.trim() | ||
| if (orchId) { | ||
| const sanitizedId = orchId.replace(/[^a-z0-9_.-]/gi, '_') | ||
| const ua = baseUserAgent ? `${baseUserAgent} ` : '' | ||
| return `${ua}actions_orchestration_id/${sanitizedId}` | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will concat the string together cause invalid user-agent format?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The concatenation should be fine — the User-Agent spec (RFC 9110 §10.1.5) is just space-separated Octokit already concatenates the same way internally ( Also worth noting this is the same approach used by |
||
| } | ||
| return baseUserAgent | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -23,6 +23,8 @@ export const GitHub = Octokit.plugin( | |||||
| paginateRest | ||||||
| ).defaults(defaults) | ||||||
|
|
||||||
| export {getUserAgentWithOrchestrationId} from './internal/utils.js' | ||||||
|
|
||||||
| /** | ||||||
| * Convience function to correctly format Octokit Options to pass into the constructor. | ||||||
|
||||||
| * Convience function to correctly format Octokit Options to pass into the constructor. | |
| * Convenience function to correctly format Octokit Options to pass into the constructor. |
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.
was already there, not related to change, good thing to fix tho
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.
should we check if base user agent already contains the orchestration ID? Or is that unlikely?