Skip to content

Commit

Permalink
Improved accuracy of Dependabot config handling; Align to observed Gi…
Browse files Browse the repository at this point in the history 
…tHub behaviour (#1536)

* Update job config to more closely match GitHub

* Fix merge issue

* More config updates

* Update unit tests

* Fix access to undefined

* Update documentation

* Fix for experiment true/false values being treated as strings instead of boolean

* Add unit tests

---------

Co-authored-by: Rhys Koedijk <rhys@koedijk.co.nz>
  • Loading branch information
@rhyskoedijk
rhyskoedijk and Rhys Koedijk authored Feb 13, 2025
1 parent e07a2e1 commit 7e79115
Show file tree
Hide file tree
Showing 4 changed files with 242 additions and 64 deletions.
165 changes: 145 additions & 20 deletions extension/tasks/dependabotV2/utils/dependabot-cli/DependabotJobBuilder.test.ts
View file
Original file line number Diff line number Diff line change
@@ -1,15 +1,20 @@
import { IDependabotGroup } from '../dependabot/interfaces/IDependabotConfig';
import { mapGroupsFromDependabotConfigToJobConfig } from './DependabotJobBuilder';
import { IDependabotGroup, IDependabotIgnoreCondition } from '../dependabot/interfaces/IDependabotConfig';
import {
mapAllowedUpdatesFromDependabotConfigToJobConfig,
mapExperiments,
mapGroupsFromDependabotConfigToJobConfig,
mapIgnoreConditionsFromDependabotConfigToJobConfig,
} from './DependabotJobBuilder';

describe('mapGroupsFromDependabotConfigToJobConfig', () => {
it('should return undefined if dependencyGroups is undefined', () => {
const result = mapGroupsFromDependabotConfigToJobConfig(undefined);
expect(result).toBeUndefined();
});

it('should return an empty array if dependencyGroups is an empty object', () => {
it('should return undefined if dependencyGroups is an empty object', () => {
const result = mapGroupsFromDependabotConfigToJobConfig({});
expect(result).toEqual([]);
expect(result).toBeUndefined();
});

it('should filter out undefined groups', () => {
Expand All @@ -21,14 +26,7 @@ describe('mapGroupsFromDependabotConfigToJobConfig', () => {
};

const result = mapGroupsFromDependabotConfigToJobConfig(dependencyGroups);
expect(result).toEqual([
{
name: 'group2',
rules: {
patterns: ['pattern2'],
},
},
]);
expect(result).toHaveLength(1);
});

it('should filter out null groups', () => {
Expand All @@ -40,14 +38,7 @@ describe('mapGroupsFromDependabotConfigToJobConfig', () => {
};

const result = mapGroupsFromDependabotConfigToJobConfig(dependencyGroups);
expect(result).toEqual([
{
name: 'group2',
rules: {
patterns: ['pattern2'],
},
},
]);
expect(result).toHaveLength(1);
});

it('should map dependency group properties correctly', () => {
Expand Down Expand Up @@ -94,3 +85,137 @@ describe('mapGroupsFromDependabotConfigToJobConfig', () => {
]);
});
});

describe('mapAllowedUpdatesFromDependabotConfigToJobConfig', () => {
it('should allow direct dependency updates if rules are undefined', () => {
const result = mapAllowedUpdatesFromDependabotConfigToJobConfig(undefined);
expect(result).toEqual([
{
'dependency-type': 'direct',
'update-type': 'all',
},
]);
});

it('should allow direct dependency security updates if rules are undefined and securityOnlyUpdate is true', () => {
const result = mapAllowedUpdatesFromDependabotConfigToJobConfig(undefined, true);
expect(result).toEqual([
{
'dependency-type': 'direct',
'update-type': 'security',
},
]);
});
});

describe('mapIgnoreConditionsFromDependabotConfigToJobConfig', () => {
it('should return undefined if rules are undefined', () => {
const result = mapIgnoreConditionsFromDependabotConfigToJobConfig(undefined);
expect(result).toBeUndefined();
});

it('should handle single version string correctly', () => {
const ignoreConditions: IDependabotIgnoreCondition[] = [
{
'dependency-name': 'dep1',
'versions': ['>1.0.0'],
},
];

const result = mapIgnoreConditionsFromDependabotConfigToJobConfig(ignoreConditions);
expect(result).toEqual([
{
'dependency-name': 'dep1',
'version-requirement': '>1.0.0',
},
]);
});

it('should handle multiple version strings correctly', () => {
const ignoreConditions: IDependabotIgnoreCondition[] = [
{
'dependency-name': 'dep1',
'versions': ['>1.0.0', '<2.0.0'],
},
];

const result = mapIgnoreConditionsFromDependabotConfigToJobConfig(ignoreConditions);
expect(result).toEqual([
{
'dependency-name': 'dep1',
'version-requirement': '>1.0.0, <2.0.0',
},
]);
});

it('should handle empty versions array correctly', () => {
const ignoreConditions: IDependabotIgnoreCondition[] = [
{
'dependency-name': 'dep1',
'versions': [],
},
];

const result = mapIgnoreConditionsFromDependabotConfigToJobConfig(ignoreConditions);
expect(result).toEqual([
{
'dependency-name': 'dep1',
'version-requirement': '',
},
]);
});
});

describe('mapExperiments', () => {
it('should return an empty object if experiments is undefined', () => {
const result = mapExperiments(undefined);
expect(result).toEqual({});
});

it('should return an empty object if experiments is an empty object', () => {
const result = mapExperiments({});
expect(result).toEqual({});
});

it('should convert string experiment value "true" to boolean `true`', () => {
const experiments = {
experiment1: 'true',
};
const result = mapExperiments(experiments);
expect(result).toEqual({
experiment1: true,
});
});

it('should convert string experiment value "false" to boolean `false`', () => {
const experiments = {
experiment1: 'false',
};
const result = mapExperiments(experiments);
expect(result).toEqual({
experiment1: false,
});
});

it('should keep boolean experiment values as is', () => {
const experiments = {
experiment1: true,
experiment2: false,
};
const result = mapExperiments(experiments);
expect(result).toEqual({
experiment1: true,
experiment2: false,
});
});

it('should keep string experiment values other than "true" or "false" as is', () => {
const experiments = {
experiment1: 'someString',
};
const result = mapExperiments(experiments);
expect(result).toEqual({
experiment1: 'someString',
});
});
});
101 changes: 70 additions & 31 deletions extension/tasks/dependabotV2/utils/dependabot-cli/DependabotJobBuilder.ts
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import {
IDependabotAllowCondition,
IDependabotGroup,
IDependabotIgnoreCondition,
IDependabotRegistry,
IDependabotUpdate,
} from '../dependabot/interfaces/IDependabotConfig';
Expand Down Expand Up @@ -112,7 +113,7 @@ export class DependabotJobBuilder {
}
}

function buildUpdateJobConfig(
export function buildUpdateJobConfig(
id: string,
taskInputs: ISharedVariables,
update: IDependabotUpdate,
Expand All @@ -122,20 +123,20 @@ function buildUpdateJobConfig(
updateDependencyNames?: string[] | undefined,
existingPullRequests?: any[],
securityVulnerabilities?: ISecurityVulnerability[],
) {
): IDependabotUpdateOperation {
const securityOnlyUpdate = update['open-pull-requests-limit'] == 0;
return {
config: update,
job: {
'id': id,
'package-manager': update['package-ecosystem'],
'update-subdependencies': true, // TODO: add config for this?
'updating-a-pull-request': updatingPullRequest || false,
'dependency-group-to-refresh': updateDependencyGroupName,
'dependency-groups': mapGroupsFromDependabotConfigToJobConfig(update.groups),
'dependencies': updateDependencyNames,
'allowed-updates': mapAllowedUpdatesFromDependabotConfigToJobConfig(update.allow),
'dependencies': updateDependencyNames?.length ? updateDependencyNames : undefined,
'allowed-updates': mapAllowedUpdatesFromDependabotConfigToJobConfig(update.allow, securityOnlyUpdate),
'ignore-conditions': mapIgnoreConditionsFromDependabotConfigToJobConfig(update.ignore),
'security-updates-only': update['open-pull-requests-limit'] == 0,
'security-updates-only': securityOnlyUpdate,
'security-advisories': mapSecurityAdvisories(securityVulnerabilities),
'source': mapSourceFromDependabotConfigToJobConfig(taskInputs, update),
'existing-pull-requests': existingPullRequests?.filter((pr) => !pr['dependency-group-name']),
Expand All @@ -146,30 +147,30 @@ function buildUpdateJobConfig(
: {
'prefix': update['commit-message']?.['prefix'],
'prefix-development': update['commit-message']?.['prefix-development'],
'include-scope': update['commit-message']?.['include'],
'include-scope':
update['commit-message']?.['include']?.toLocaleLowerCase()?.trim() == 'scope' ? true : undefined,
},
'experiments': Object.keys(taskInputs.experiments || {}).reduce(
(acc, key) => {
// Replace '-' with '_' in the experiment keys to match the dependabot-core models
acc[key.replace(/-/g, '_')] = taskInputs.experiments[key];
return acc;
},
{} as Record<string, string | boolean>,
),
'max-updater-run-time': undefined, // TODO: add config for this?
'reject-external-code': update['insecure-external-code-execution']?.toLocaleLowerCase() == 'allow',
'experiments': mapExperiments(taskInputs.experiments),
'reject-external-code': update['insecure-external-code-execution']?.toLocaleLowerCase()?.trim() == 'allow',
'repo-private': undefined, // TODO: add config for this?
'repo-contents-path': undefined, // TODO: add config for this?
'requirements-update-strategy': mapVersionStrategyToRequirementsUpdateStrategy(update['versioning-strategy']),
'lockfile-only': update['versioning-strategy'] === 'lockfile-only',
'vendor-dependencies': update.vendor,
'debug': taskInputs.debug,

// TODO: Investigate if these options are needed or are now obsolete.
// These options don't appear to be used by dependabot-core yet/anymore,
// but do appear in GitHub Dependabot job logs seen in the wild.
//'max-updater-run-time': 2700,
//'proxy-log-response-body-on-auth-failure': true,
//'update-subdependencies': false,
},
credentials: mapRegistryCredentialsFromDependabotConfigToJobConfig(taskInputs, registries),
};
}

function mapSourceFromDependabotConfigToJobConfig(taskInputs: ISharedVariables, update: IDependabotUpdate): any {
export function mapSourceFromDependabotConfigToJobConfig(taskInputs: ISharedVariables, update: IDependabotUpdate): any {
return {
'provider': 'azure',
'api-endpoint': taskInputs.apiEndpointUrl,
Expand All @@ -182,8 +183,10 @@ function mapSourceFromDependabotConfigToJobConfig(taskInputs: ISharedVariables,
};
}

export function mapGroupsFromDependabotConfigToJobConfig(dependencyGroups: Record<string, IDependabotGroup>): any[] {
if (!dependencyGroups) {
export function mapGroupsFromDependabotConfigToJobConfig(
dependencyGroups: Record<string, IDependabotGroup>,
): any[] | undefined {
if (!dependencyGroups || !Object.keys(dependencyGroups).length) {
return undefined;
}
return Object.keys(dependencyGroups)
Expand All @@ -206,36 +209,53 @@ export function mapGroupsFromDependabotConfigToJobConfig(dependencyGroups: Recor
.filter((g) => g);
}

function mapAllowedUpdatesFromDependabotConfigToJobConfig(allowedUpdates: IDependabotAllowCondition[]): any[] {
export function mapAllowedUpdatesFromDependabotConfigToJobConfig(
allowedUpdates: IDependabotAllowCondition[],
securityOnlyUpdate?: boolean,
): any[] {
// If no allow conditions are specified, update direct dependencies by default; This is what GitHub does.
// NOTE: 'update-type' appears to be a deprecated config, but still appears in the dependabot-core model and GitHub Dependabot job logs.
// See: https://github.com/dependabot/dependabot-core/blob/b3a0c1f86c20729494097ebc695067099f5b4ada/updater/lib/dependabot/job.rb#L253C1-L257C78
if (!allowedUpdates) {
// If no allow conditions are specified, update all dependencies by default
return [{ 'dependency-type': 'all' }];
return [
{
'dependency-type': 'direct',
'update-type': securityOnlyUpdate ? 'security' : 'all',
},
];
}
return allowedUpdates.map((allow) => {
return {
'dependency-name': allow['dependency-name'],
'dependency-type': allow['dependency-type'],
//'update-type': allow["update-type"] // TODO: This is missing from dependabot.ymal docs, but is used in the dependabot-core job model!?
'update-type': allow['update-type'],
};
});
}

function mapIgnoreConditionsFromDependabotConfigToJobConfig(ignoreConditions: IDependabotAllowCondition[]): any[] {
export function mapIgnoreConditionsFromDependabotConfigToJobConfig(
ignoreConditions: IDependabotIgnoreCondition[],
): any[] {
if (!ignoreConditions) {
return undefined;
}
return ignoreConditions.map((ignore) => {
return {
'source': ignore['source'],
'updated-at': ignore['updated-at'],
'dependency-name': ignore['dependency-name'],
//'source': ignore["source"], // TODO: This is missing from dependabot.ymal docs, but is used in the dependabot-core job model!?
'update-types': ignore['update-types'],
//'updated-at': ignore["updated-at"], // TODO: This is missing from dependabot.ymal docs, but is used in the dependabot-core job model!?
'version-requirement': (<string[]>ignore['versions'])?.join(', '), // TODO: Test this, not sure how this should be parsed...

// The dependabot.yml config docs are not very clear about acceptable values; after scanning dependabot-core and dependabot-cli,
// this could either be a single version string (e.g. '>1.0.0'), or multiple version strings separated by commas (e.g. '>1.0.0, <2.0.0')
'version-requirement': Array.isArray(ignore['versions'])
? (<string[]>ignore['versions'])?.join(', ')
: <string>ignore['versions'],
};
});
}

function mapSecurityAdvisories(securityVulnerabilities: ISecurityVulnerability[]): any[] {
export function mapSecurityAdvisories(securityVulnerabilities: ISecurityVulnerability[]): any[] {
if (!securityVulnerabilities) {
return undefined;
}
Expand All @@ -260,7 +280,7 @@ function mapSecurityAdvisories(securityVulnerabilities: ISecurityVulnerability[]
});
}

function mapVersionStrategyToRequirementsUpdateStrategy(versioningStrategy: string): string | undefined {
export function mapVersionStrategyToRequirementsUpdateStrategy(versioningStrategy: string): string | undefined {
if (!versioningStrategy) {
return undefined;
}
Expand All @@ -280,7 +300,7 @@ function mapVersionStrategyToRequirementsUpdateStrategy(versioningStrategy: stri
}
}

function mapRegistryCredentialsFromDependabotConfigToJobConfig(
export function mapRegistryCredentialsFromDependabotConfigToJobConfig(
taskInputs: ISharedVariables,
registries: Record<string, IDependabotRegistry>,
): any[] {
Expand Down Expand Up @@ -322,3 +342,22 @@ function mapRegistryCredentialsFromDependabotConfigToJobConfig(

return registryCredentials;
}

export function mapExperiments(experiments: Record<string, string | boolean>): Record<string, string | boolean> {
return Object.keys(experiments || {}).reduce(
(acc, key) => {
// Experiment values are known to be either 'true', 'false', or a string value.
// If the value is 'true' or 'false', convert it to a boolean type so that dependabot-core handles it correctly.
const value = experiments[key];
if (typeof value === 'string' && value?.toLocaleLowerCase() === 'true') {
acc[key] = true;
} else if (typeof value === 'string' && value?.toLocaleLowerCase() === 'false') {
acc[key] = false;
} else {
acc[key] = value;
}
return acc;
},
{} as Record<string, string | boolean>,
);
}
Loading

0 comments on commit 7e79115

Please sign in to comment.