Skip to content
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

fix: add missing noop param for full-sync #733

Open
wants to merge 1 commit into
base: main-enterprise
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
33 changes: 21 additions & 12 deletions full-sync.js
Original file line number Diff line number Diff line change
@@ -1,19 +1,28 @@
const { createProbot } = require('probot')
const appFn = require('./')
const { FULL_SYNC_NOOP } = require('./lib/env')
const { createProbot } = require('probot')

async function performFullSync (appFn, noop) {
const probot = createProbot()
probot.log.info(`Starting full sync with NOOP=${noop}`)

const probot = createProbot()
probot.log.info('Starting full sync.')
const app = appFn(probot, {})
app.syncInstallation()
.then(settings => {
if (settings.errors.length > 0) {
try {
const app = appFn(probot, {})
const settings = await app.syncInstallation(noop)

if (settings.errors && settings.errors.length > 0) {
probot.log.error('Errors occurred during full sync.')
process.exit(1)
} else {
probot.log.info('Done with full sync.')
}
})
.catch(error => {

probot.log.info('Full sync completed successfully.')
} catch (error) {
process.stdout.write(`Unexpected error during full sync: ${error}\n`)
process.exit(1)
})
}
}

performFullSync(appFn, FULL_SYNC_NOOP).catch((error) => {
console.error('Fatal error during full sync:', error)
process.exit(1)
})
4 changes: 2 additions & 2 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) =>
}
}

async function syncInstallation () {
async function syncInstallation (noop = false) {
robot.log.trace('Fetching installations')
const github = await robot.auth()

Expand All @@ -249,7 +249,7 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) =>
log: robot.log,
repo: () => { return { repo: env.ADMIN_REPO, owner: installation.account.login } }
}
return syncAllSettings(false, context)
return syncAllSettings(noop, context)
}
return null
}
Expand Down
3 changes: 2 additions & 1 deletion lib/env.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,6 @@ module.exports = {
DEPLOYMENT_CONFIG_FILE: process.env.DEPLOYMENT_CONFIG_FILE || 'deployment-settings.yml',
CREATE_PR_COMMENT: process.env.CREATE_PR_COMMENT || 'true',
CREATE_ERROR_ISSUE: process.env.CREATE_ERROR_ISSUE || 'true',
BLOCK_REPO_RENAME_BY_HUMAN: process.env.BLOCK_REPO_RENAME_BY_HUMAN || 'false'
BLOCK_REPO_RENAME_BY_HUMAN: process.env.BLOCK_REPO_RENAME_BY_HUMAN || 'false',
FULL_SYNC_NOOP: process.env.FULL_SYNC_NOOP === 'true'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@decyjphr

We have different implementations for boolean environment variables.
For example, CREATE_PR_COMMENT is handled as a string, while with this changes FULL_SYNC_NOOP is implemented as a boolean value. This is reflected in the different approaches in our unit tests.

I think it would be beneficial to standardize how we handle boolean environment variables across the project. Once we decide on the preferred implementation (string or boolean), I can create a new PR to refactor the existing code for consistency.
I can also update my changes to fit current implementation.

}
8 changes: 8 additions & 0 deletions test/unit/lib/env.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,11 @@ describe('env', () => {
const CREATE_PR_COMMENT = envTest.CREATE_PR_COMMENT
expect(CREATE_PR_COMMENT).toEqual('true')
})

it('loads default FULL_SYNC_NOOP if not passed', () => {
const FULL_SYNC_NOOP = envTest.FULL_SYNC_NOOP
expect(FULL_SYNC_NOOP).toEqual(false)
})
})

describe('load override values', () => {
Expand All @@ -37,6 +42,7 @@ describe('env', () => {
process.env.SETTINGS_FILE_PATH = 'safe-settings.yml'
process.env.DEPLOYMENT_CONFIG_FILE = 'safe-settings-deployment.yml'
process.env.CREATE_PR_COMMENT = 'false'
process.env.FULL_SYNC_NOOP = false
})

it('loads override values if passed', () => {
Expand All @@ -51,6 +57,8 @@ describe('env', () => {
expect(DEPLOYMENT_CONFIG_FILE).toEqual('safe-settings-deployment.yml')
const CREATE_PR_COMMENT = envTest.CREATE_PR_COMMENT
expect(CREATE_PR_COMMENT).toEqual('false')
const FULL_SYNC_NOOP = envTest.FULL_SYNC_NOOP
expect(FULL_SYNC_NOOP).toEqual(false)
})
})
})