Skip to content
This repository has been archived by the owner on May 20, 2023. It is now read-only.

Ignore pinned issues #266

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
65 changes: 63 additions & 2 deletions lib/stale.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ module.exports = class Stale {
}
}

getStale (type) {
async getStale (type) {
const onlyLabels = this.getConfigValue(type, 'onlyLabels')
const staleLabel = this.getConfigValue(type, 'staleLabel')
const exemptLabels = this.getConfigValue(type, 'exemptLabels')
Expand All @@ -80,7 +80,68 @@ module.exports = class Stale {

const query = queryParts.join(' ')
const days = this.getConfigValue(type, 'days') || this.getConfigValue(type, 'daysUntilStale')
return this.search(type, days, query)
const results = await this.search(type, days, query)

const pinned = await this.getPinnedNumbers(type)
const nonPinned = results.data.items.filter(issue => pinned.includes(issue.number))
return {
...results,
data: {
...results.data,
total_count: nonPinned.length,
items: nonPinned,
}
}
Comment on lines +87 to +94
Copy link
Author

Choose a reason for hiding this comment

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

I opted to preserve any untouched portions of the return payload, rather than just return {data:{items: nonPinned}}, despite the fact they aren't used.

It felt weird to change the return type, rather than just the contents.

}

async getPinnedNumbers(type) {
Copy link
Author

Choose a reason for hiding this comment

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

We could potentially add a config setting to control if pinned issues should be ignored or not, but I wasn't sure if it was worth it.

if (type === 'pulls') {
// Pull Requests cannot be pinned
return []
} else if (type !== 'issues') {
throw new Error(`Unknown type: ${type}. Valid types are 'pulls' and 'issues'`)
}

const { owner, repo } = this.config

try {
// GitHub's v3 REST API doesn't support Pinned Issues; v4 GraphQL API does
const {data, errors} = await this.github.graphql(
`
query Issues($owner: String!, $repo: String!) {
repository(owner: $owner, name: $repo) {
pinnedIssues(first: 100) {
nodes {
number
}
pageInfo {
# This should always be false, as only 3 issues can be pinned at the same time
hasNextPage
}
Comment on lines +117 to +120
Copy link
Author

Choose a reason for hiding this comment

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

This could go away, but I felt it was was useful to document why we don't have to paginate.

}
}
}
`,
Comment on lines +110 to +124
Copy link
Author

Choose a reason for hiding this comment

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

I originally extracted this into a constant, but decided it might be clearer to have it nearby.

{
owner,
repo,
headers: {
// Opt-in to Pinned Issues API preview
accept: 'application/vnd.github.elektra-preview+json'
},
},
)

if (errors && errors.length) {
throw new Error(errors[0].message)
}

return data.repository.pinnedIssues.nodes.map(issue => issue.number)
} catch (error) {
this.logger.error(`Encountered an error while excluding pinned items for ${owner}/${repo}: ${error}`)
Copy link
Author

Choose a reason for hiding this comment

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

I wasn't sure if this was the best way to log errors, but I think it's important that the bot not break if the Pinned Issues Preview API changes.

// In the event of an error, proceed as if no pinned issues found
return []
}
}

getClosable (type) {
Expand Down
8 changes: 8 additions & 0 deletions test/stale.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -195,4 +195,12 @@ describe('stale', () => {
expect(stale.getClosable).not.toHaveBeenCalled()
}
)

test('should ignore pinned issues', () => {
expect('Test implementation pending').toStrictEqual('Test implemented')
})

test('should log, but ignore errors fetching pinned issues', () => {
expect('Test implementation pending').toStrictEqual('Test implemented')
})
})