Skip to content

feat: Cache key uses repo.org only if USE_CONFIG_FROM_PULL_REQUEST is disabled. #775

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

Open
wants to merge 4 commits into
base: master
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
77 changes: 77 additions & 0 deletions __tests__/unit/configuration/configuration.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -762,6 +762,83 @@ describe('with version 1', () => {
config = await Configuration.fetchConfigFile(context)
expect(config.mergeable.length).toEqual(1)
expect(config.mergeable[0].name).toBe('repository rules')

// Clean-up the cache state after test
const configCache = Configuration.getCache()
await configCache.reset()
})

test('config cache uses only owner as cache key if only global config can be fetched', async () => {
const orgConfigStr = `
mergeable:
issues:
stale:
days: 20
message: Issue test
pull_requests:
stale:
days: 20
message: PR test
`
// Check that on first load, the cache is correctly set
const emptyConfig = '{}'
const orgConfig = yaml.safeLoad(orgConfigStr)
const context = createMockGhConfig(emptyConfig, orgConfigStr)
context.globalSettings.use_config_cache = true
context.globalSettings.use_config_from_pull_request = false
context.globalSettings.use_org_as_default_config = true
const configCache = Configuration.getCache()
const repo = context.repo()
let keys = await configCache.keys()
expect(keys.length).toEqual(0)
context.eventName = 'push'
context.payload.head_commit = { added: ['.github/mergeable.yml'] }
expect(context.probotContext.config.mock.calls.length).toEqual(0)
let config = await Configuration.fetchConfigFile(context)
expect(config).toEqual(orgConfig)
keys = await configCache.keys()
expect(keys.length).toEqual(1)
const cachedConfig = await configCache.get(repo.owner)
expect(cachedConfig).toEqual(orgConfig)
// Check that cached value in the repo.owner is really used
const injectedConfig = { test: 'test' }
await configCache.set(repo.owner, injectedConfig)
config = await Configuration.fetchConfigFile(context)
expect(config).toEqual(injectedConfig)

// Clean-up the cache state after test
await configCache.reset()
})

test('cache is reset for org correctly when .github repo mergeable file is modified', async () => {
const orgConfigStr = `
mergeable:
pull_requests:
stale:
days: 20
`
const orgConfig = yaml.safeLoad(orgConfigStr)
const context = createMockGhConfig('{}', orgConfigStr)
context.globalSettings.use_config_cache = true
context.globalSettings.use_config_from_pull_request = false
context.globalSettings.use_org_as_default_config = true
const repo = context.repo()
const configCache = Configuration.getCache()
// Inject config that will be reset and overriden with the actual orgConfig
const injectedConfig = { test: 'test' }
configCache.set(repo.owner, injectedConfig)
let keys = await configCache.keys()
expect(keys.length).toEqual(1)
context.eventName = 'push'
context.repo = jest.fn().mockReturnValue({ owner: repo.owner, repo: '.github' })
context.payload.head_commit = { added: ['.github/mergeable.yml'] }
const config = await Configuration.fetchConfigFile(context)
expect(config).toEqual(orgConfig)
keys = await configCache.keys()
expect(keys.length).toEqual(1)

// Clean-up the cache state after test
await configCache.reset()
})
})

Expand Down
12 changes: 10 additions & 2 deletions lib/configuration/configuration.js
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,12 @@ class Configuration {
const repo = context.repo()

if (repo.repo === '.github') {
// Only org cache needs to be deleted if use_config_from_pull_request is disabled
if (globalSettings.use_config_from_pull_request !== undefined && globalSettings.use_config_from_pull_request === false) {
cacheManager.del(repo.owner)
return
}

const keys = await cacheManager.keys()
keys.filter(key => key.startsWith(`${repo.owner}/`)).map(key => cacheManager.del(key))
} else {
Expand All @@ -120,8 +126,10 @@ class Configuration {
// using pull request configuration by default, this behaviour
// can be controlled using USE_CONFIG_FROM_PULL_REQUEST environment variable
let usePullRequestConfig = true
let cacheKey = `${repo.owner}/${repo.repo}`
if (globalSettings.use_config_from_pull_request !== undefined && globalSettings.use_config_from_pull_request === false) {
usePullRequestConfig = false
cacheKey = `${repo.owner}`
}
if (usePullRequestConfig && ['pull_request', 'pull_request_review'].includes(context.eventName)) {
const payload = context.payload.pull_request
Expand Down Expand Up @@ -157,7 +165,7 @@ class Configuration {

if (globalSettings.use_config_cache !== undefined && globalSettings.use_config_cache === true) {
Configuration.resetCache(context)
config = await cacheManager.get(`${repo.owner}/${repo.repo}`)
config = await cacheManager.get(cacheKey)
if (config) {
return config
}
Expand All @@ -181,7 +189,7 @@ class Configuration {
}

if (globalSettings.use_config_cache !== undefined && globalSettings.use_config_cache === true) {
cacheManager.set(`${repo.owner}/${repo.repo}`, config)
cacheManager.set(cacheKey, config)
}

return config
Expand Down