From 6e9c73e35a1447af27bbe6d61e3c1d8c1d07f1c4 Mon Sep 17 00:00:00 2001
From: Rhys Arkins <rhys@keylocation.sg>
Date: Mon, 28 Aug 2017 15:50:11 +0200
Subject: [PATCH] feat: restart repo renovate after automerge (#751)
Previously, the automerge feature was causing some undesirable behaviour when multiple branches were open at the same time. Example: #707. The main problem is that other branches will still be calculated based on the original `package.json` contents and not the post-merge contents. The simplest solution seems to be:
- Stop all subsequent branch processing after any automerge
- Restart repository renovation
This continues until no branch has automerged in a cycle.
Closes #750
---
lib/workers/branch/index.js | 19 +--
lib/workers/pr/index.js | 13 +-
lib/workers/repository/index.js | 176 ++++++++++++++------------
test/workers/branch/index.spec.js | 1 +
test/workers/repository/index.spec.js | 24 +++-
5 files changed, 139 insertions(+), 94 deletions(-)
diff --git a/lib/workers/branch/index.js b/lib/workers/branch/index.js
index b65b85e198..fce07cf9b4 100644
--- a/lib/workers/branch/index.js
+++ b/lib/workers/branch/index.js
@@ -30,14 +30,14 @@ async function processBranch(branchConfig) {
// Check schedule
if (!isScheduledNow(config)) {
logger.info('Skipping branch as it is not scheduled');
- return;
+ return 'not-scheduled';
}
logger.info(`Branch has ${dependencies.length} upgrade(s)`);
if (await prAlreadyExisted(config)) {
logger.info('Closed PR already exists. Skipping branch.');
- return;
+ return 'already-existed';
}
config.parentBranch = await getParentBranch(config);
logger.debug(`Using parentBranch: ${config.parentBranch}`);
@@ -67,7 +67,7 @@ async function processBranch(branchConfig) {
// Return now if no branch exists
if ((await config.api.branchExists(config.branchName)) === false) {
logger.debug('Branch does not exist - returning');
- return;
+ return 'no-branch';
}
// Set branch statuses
@@ -78,7 +78,7 @@ async function processBranch(branchConfig) {
const branchMerged = await tryBranchAutomerge(config);
if (branchMerged) {
logger.debug('Branch is automerged - returning');
- return;
+ return 'automerged';
}
} catch (err) {
if (err.message !== 'lockFileError') {
@@ -87,7 +87,7 @@ async function processBranch(branchConfig) {
logger.info('Error updating branch');
}
// Don't throw here - we don't want to stop the other renovations
- return;
+ return 'error';
}
try {
logger.debug('Ensuring PR');
@@ -98,11 +98,14 @@ async function processBranch(branchConfig) {
const pr = await prWorker.ensurePr(config);
// TODO: ensurePr should check for automerge itself
if (pr) {
- await prWorker.checkAutoMerge(pr, config);
+ const prAutomerged = await prWorker.checkAutoMerge(pr, config);
+ if (prAutomerged) {
+ return 'automerged';
+ }
}
} catch (err) {
logger.error({ err }, `Error ensuring PR: ${err.message}`);
+ // Don't throw here - we don't want to stop the other renovations
}
-
- // Don't throw here - we don't want to stop the other renovations
+ return 'done';
}
diff --git a/lib/workers/pr/index.js b/lib/workers/pr/index.js
index 63879f19c3..eea336d0ef 100644
--- a/lib/workers/pr/index.js
+++ b/lib/workers/pr/index.js
@@ -207,11 +207,11 @@ async function checkAutoMerge(pr, config) {
// Return if PR not ready for automerge
if (pr.mergeable !== true) {
logger.info('PR is not mergeable');
- return;
+ return false;
}
if (config.requiredStatusChecks && pr.mergeable_state === 'unstable') {
logger.info('PR mergeable state is unstable');
- return;
+ return false;
}
// Check branch status
const branchStatus = await config.api.getBranchStatus(
@@ -221,17 +221,18 @@ async function checkAutoMerge(pr, config) {
logger.debug(`branchStatus=${branchStatus}`);
if (branchStatus !== 'success') {
logger.info('Branch status is not "success"');
- return;
+ return false;
}
// Check if it's been touched
if (!pr.canRebase) {
logger.info('PR is ready for automerge but has been modified');
- return;
+ return false;
}
// Let's merge this
logger.info(`Automerging #${pr.number}`);
await config.api.mergePr(pr);
- } else {
- logger.debug('No automerge');
+ return true;
}
+ logger.debug('No automerge');
+ return false;
}
diff --git a/lib/workers/repository/index.js b/lib/workers/repository/index.js
index 23c08f0433..fd603979b4 100644
--- a/lib/workers/repository/index.js
+++ b/lib/workers/repository/index.js
@@ -20,67 +20,29 @@ async function renovateRepository(repoConfig, token) {
config.warnings = [];
logger.trace({ config }, 'renovateRepository');
try {
- config = await apis.initApis(config, token);
- config = await apis.mergeRenovateJson(config);
- if (config.enabled === false) {
- logger.debug('repository is disabled');
- await cleanup.pruneStaleBranches(config, []);
- return;
- }
- if (config.isFork && !config.renovateJsonPresent) {
- logger.debug('repository is a fork and not manually configured');
- return;
- }
- if (config.baseBranch) {
- // Renovate should read content and target PRs here
- if (await config.api.branchExists(config.baseBranch)) {
- config.api.setBaseBranch(config.baseBranch);
- } else {
- // Warn and ignore setting (use default branch)
- const message = `The configured baseBranch "${config.baseBranch}" is not present. Ignoring`;
- config.errors.push({
- depName: 'baseBranch',
- message,
- });
- logger.warn(message);
- }
- }
- // Detect package files in default branch if not manually provisioned
- if (config.packageFiles.length === 0) {
- logger.debug('Detecting package files');
- config = await apis.detectPackageFiles(config);
- // If we can't detect any package.json then return
- if (config.packageFiles.length === 0) {
- logger.info('Cannot detect package.json');
+ let branchList;
+ let baseBranchUpdated;
+ let loopCount = 1;
+ do {
+ logger.debug(`renovateRepository loop ${loopCount}`);
+ baseBranchUpdated = false;
+ config = await apis.initApis(config, token);
+ config = await apis.mergeRenovateJson(config);
+ if (config.enabled === false) {
+ logger.debug('repository is disabled');
+ await cleanup.pruneStaleBranches(config, []);
return;
}
- logger.debug(
- `Detected ${config.packageFiles
- .length} package files: ${config.packageFiles}`
- );
- }
- logger.debug('Resolving package files and content');
- config = await apis.resolvePackageFiles(config);
- logger.trace({ config }, 'post-packageFiles config');
- // TODO: why is this fix needed?!
- config.logger = logger;
- config.repoIsOnboarded = await onboarding.getOnboardingStatus(config);
- if (!config.repoIsOnboarded) {
- config.contentBaseBranch = `${config.branchPrefix}configure`;
- // Remove packageFile list in case they are provisioned in renovate.json
- const packageFiles = config.packageFiles.map(
- packageFile => packageFile.packageFile
- );
- config.packageFiles = [];
- config = await apis.mergeRenovateJson(config, config.contentBaseBranch);
- // Restore previous packageFile list if not provisioned manually
- if (config.packageFiles.length === 0) {
- config.packageFiles = packageFiles;
+ if (config.isFork && !config.renovateJsonPresent) {
+ logger.debug('repository is a fork and not manually configured');
+ return;
}
if (config.baseBranch) {
+ // Renovate should read content and target PRs here
if (await config.api.branchExists(config.baseBranch)) {
- config.contentBaseBranch = config.baseBranch;
+ config.api.setBaseBranch(config.baseBranch);
} else {
+ // Warn and ignore setting (use default branch)
const message = `The configured baseBranch "${config.baseBranch}" is not present. Ignoring`;
config.errors.push({
depName: 'baseBranch',
@@ -89,34 +51,90 @@ async function renovateRepository(repoConfig, token) {
logger.warn(message);
}
}
+ // Detect package files in default branch if not manually provisioned
+ if (config.packageFiles.length === 0) {
+ logger.debug('Detecting package files');
+ config = await apis.detectPackageFiles(config);
+ // If we can't detect any package.json then return
+ if (config.packageFiles.length === 0) {
+ logger.info('Cannot detect package.json');
+ return;
+ }
+ logger.debug(
+ `Detected ${config.packageFiles
+ .length} package files: ${config.packageFiles}`
+ );
+ }
+ logger.debug('Resolving package files and content');
config = await apis.resolvePackageFiles(config);
- config = await presets.resolveConfigPresets(config);
+ logger.trace({ config }, 'post-packageFiles config');
+ // TODO: why is this fix needed?!
config.logger = logger;
- logger.trace({ config }, 'onboarding config');
- }
- const allUpgrades = await upgrades.determineRepoUpgrades(config);
- const res = await upgrades.branchifyUpgrades(allUpgrades, logger);
- config.errors = config.errors.concat(res.errors);
- config.warnings = config.warnings.concat(res.warnings);
- const branchUpgrades = res.upgrades;
- logger.debug(`Updating ${branchUpgrades.length} branch(es)`);
- logger.trace({ config: branchUpgrades }, 'branchUpgrades');
- let branchList;
- if (config.repoIsOnboarded) {
- for (const branchUpgrade of branchUpgrades) {
- await branchWorker.processBranch(
- branchUpgrade,
- config.errors,
- config.warnings
+ config.repoIsOnboarded = await onboarding.getOnboardingStatus(config);
+ if (!config.repoIsOnboarded) {
+ config.contentBaseBranch = `${config.branchPrefix}configure`;
+ // Remove packageFile list in case they are provisioned in renovate.json
+ const packageFiles = config.packageFiles.map(
+ packageFile => packageFile.packageFile
);
+ config.packageFiles = [];
+ config = await apis.mergeRenovateJson(config, config.contentBaseBranch);
+ // Restore previous packageFile list if not provisioned manually
+ if (config.packageFiles.length === 0) {
+ config.packageFiles = packageFiles;
+ }
+ if (config.baseBranch) {
+ if (await config.api.branchExists(config.baseBranch)) {
+ config.contentBaseBranch = config.baseBranch;
+ } else {
+ const message = `The configured baseBranch "${config.baseBranch}" is not present. Ignoring`;
+ config.errors.push({
+ depName: 'baseBranch',
+ message,
+ });
+ logger.warn(message);
+ }
+ }
+ config = await apis.resolvePackageFiles(config);
+ config = await presets.resolveConfigPresets(config);
+ config.logger = logger;
+ logger.trace({ config }, 'onboarding config');
}
- branchList = branchUpgrades.map(upgrade => upgrade.branchName);
- logger.debug(`branchList=${branchList}`);
- } else {
- await onboarding.ensurePr(config, branchUpgrades);
- logger.info('"Configure Renovate" PR needs to be closed first');
- branchList = [`${config.branchPrefix}configure`];
- }
+ const allUpgrades = await upgrades.determineRepoUpgrades(config);
+ const res = await upgrades.branchifyUpgrades(allUpgrades, logger);
+ config.errors = config.errors.concat(res.errors);
+ config.warnings = config.warnings.concat(res.warnings);
+ const branchUpgrades = res.upgrades;
+ logger.debug(`Updating ${branchUpgrades.length} branch(es)`);
+ logger.trace({ config: branchUpgrades }, 'branchUpgrades');
+ if (config.repoIsOnboarded) {
+ for (const branchUpgrade of branchUpgrades) {
+ if (!baseBranchUpdated) {
+ const branchResult = await branchWorker.processBranch(
+ branchUpgrade,
+ config.errors,
+ config.warnings
+ );
+ if (branchResult === 'automerged') {
+ // Stop procesing other branches because base branch has been changed by an automerge
+ logger.info('Restarting repo renovation after automerge');
+ baseBranchUpdated = true;
+ }
+ } else {
+ logger.debug(
+ `Skipping branchUpgrade as base branch has been modified`
+ );
+ }
+ }
+ branchList = branchUpgrades.map(upgrade => upgrade.branchName);
+ logger.debug(`branchList=${branchList}`);
+ } else {
+ await onboarding.ensurePr(config, branchUpgrades);
+ logger.info('"Configure Renovate" PR needs to be closed first');
+ branchList = [`${config.branchPrefix}configure`];
+ }
+ loopCount += 1;
+ } while (baseBranchUpdated);
await cleanup.pruneStaleBranches(config, branchList);
} catch (err) {
// Swallow this error so that other repositories can be processed
diff --git a/test/workers/branch/index.spec.js b/test/workers/branch/index.spec.js
index 40d9f70f84..b1d8d10bc1 100644
--- a/test/workers/branch/index.spec.js
+++ b/test/workers/branch/index.spec.js
@@ -79,6 +79,7 @@ describe('workers/branch', () => {
config.api.branchExists.mockReturnValueOnce(true);
automerge.tryBranchAutomerge.mockReturnValueOnce(false);
prWorker.ensurePr.mockReturnValueOnce({});
+ prWorker.checkAutoMerge.mockReturnValueOnce(true);
await branchWorker.processBranch(config);
expect(prWorker.ensurePr.mock.calls).toHaveLength(1);
expect(prWorker.checkAutoMerge.mock.calls).toHaveLength(1);
diff --git a/test/workers/repository/index.spec.js b/test/workers/repository/index.spec.js
index c452d92fcc..eeeb26b130 100644
--- a/test/workers/repository/index.spec.js
+++ b/test/workers/repository/index.spec.js
@@ -20,7 +20,7 @@ describe('workers/repository', () => {
onboarding.ensurePr = jest.fn();
upgrades.determineRepoUpgrades = jest.fn(() => []);
upgrades.branchifyUpgrades = jest.fn(() => ({ branchUpgrades: {} }));
- branchWorker.processBranch = jest.fn(() => 'some-branch');
+ branchWorker.processBranch = jest.fn(() => 'done');
config = {
lockFileMaintenance: true,
api: {
@@ -153,6 +153,28 @@ describe('workers/repository', () => {
expect(branchWorker.processBranch.mock.calls.length).toBe(3);
expect(config.logger.error.mock.calls.length).toBe(0);
});
+ it('skips branchWorker after automerging', async () => {
+ config.packageFiles = ['package.json'];
+ config.hasRenovateJson = true;
+ onboarding.getOnboardingStatus.mockReturnValue(true);
+ upgrades.branchifyUpgrades.mockReturnValueOnce({
+ upgrades: [{}, {}, {}],
+ });
+ upgrades.branchifyUpgrades.mockReturnValueOnce({
+ upgrades: [{}, {}],
+ });
+ upgrades.branchifyUpgrades.mockReturnValueOnce({
+ upgrades: [{}],
+ });
+ upgrades.branchifyUpgrades.mockReturnValueOnce({
+ upgrades: [],
+ });
+ branchWorker.processBranch.mockReturnValue('automerged');
+ await repositoryWorker.renovateRepository(config);
+ expect(upgrades.branchifyUpgrades.mock.calls).toHaveLength(4);
+ expect(branchWorker.processBranch.mock.calls).toHaveLength(3);
+ expect(config.logger.error.mock.calls).toHaveLength(0);
+ });
it('swallows errors', async () => {
apis.initApis.mockImplementationOnce(() => {
throw new Error('bad init');
--
GitLab