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

include-paths: Permit filtering only PRs that modify path prefixes #1084

Merged
merged 4 commits into from Mar 3, 2022

Conversation

blast-hardcheese
Copy link
Contributor

@blast-hardcheese blast-hardcheese commented Feb 25, 2022

This PR implements part of #935


This introduces a new GraphQL query, requesting all node IDs that contain modifications to the specified file list. That node list is then used to filter the full set of PRs to only the set that contain those node IDs, thus giving us only the PRs that include changes to our paths.

Examples of include-paths:

- src/main
- README.md
- doc/

For a project with ancillary files that need not be considered for a release (an automated job that regenerates binary files on an automated basis, generating lots of noise, for example).

In my case, this is because I have many parallel instances of release-drafter, each running with different configurations, to release different binary artifacts from a single source tree. The project is organized this way for ease of contribution, as well as reducing needless version bumps for consumers (where different source trees contain completely unrelated changesets).

lib/commits.js Outdated
@@ -2,6 +2,34 @@ const _ = require('lodash')
const { log } = require('./log')
const { paginate } = require('./pagination')

const findCommitsWithPathChangesQuery = ({ includePaths }) => /* GraphQL */ `
Copy link
Member

@jetersen jetersen Feb 25, 2022

Choose a reason for hiding this comment

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

Could this be in the existing query?

or are there benefits to using a separate query for this?

Should be possible with directives: https://graphql.org/learn/queries/#directives

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've done a bit of research into this and don't see a way to do this without inlining.
history(path: String, ... includes the following documentation:

path (String)

If non-null, filters history to only show commits touching files under this path.

which doesn't support an array. Attempting to unroll this and do the filter in JS would be possible, but instead of issuing an individual GraphQL query, we'd end up needing to round-trip to the API for each desired path.

What could be done is to transform the whole ... on Commit in the main findCommitsWithAssociatedPullRequests into a fragment, parameterized by path, where if includePaths is an empty array we just pass null.

The downside there is that we'd then need to interpolate the resulting array of results ourselves, instead of what the current implementation does, selecting a group of node IDs that can be used to filter the array of returned PRs without altering the order.

I'm by far not an expert on either GraphQL itself, or GitHub's exposed GraphQL API, so I may have missed something and would be quite happy to revisit this approach, string-interpolating into a GraphQL query is undesirable from a number of perspectives, I just didn't see a way to do it without asking for support for this functionality from GitHub themselves.

Thanks for taking a look

test/index.test.js Outdated Show resolved Hide resolved
@blast-hardcheese blast-hardcheese force-pushed the include-paths branch 2 times, most recently from 16afe74 to 4e67daf Compare March 2, 2022 20:18
@@ -0,0 +1,6 @@
template: Placeholder with example. Automatically calculated values based on previous releases are next major=$NEXT_MAJOR_VERSION, minor=$NEXT_MINOR_VERSION, patch=$NEXT_PATCH_VERSION. Manual input version is $INPUT_VERSION.
Copy link
Member

@jetersen jetersen Mar 3, 2022

Choose a reason for hiding this comment

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

How about including $CHANGES into the template, I assume a PR made it into the changes block? 🤔

Properly better to use something like:

template: |
  # What's Changed
  $CHANGES

The test is not really asserting on the release drafting part 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That clicked what was missing about my tests, hah! Finally got the snapshot tester working as well, I actually believe this PR is good, other than style or structure nits. The functionality is there, I've tested it in my own test harness.

Copy link
Member

Choose a reason for hiding this comment

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

Luckily style and structure is taken care by our js tooling as well 👏 eslint, prettier, lint-staged and husky 👏

@blast-hardcheese blast-hardcheese mentioned this pull request Mar 3, 2022
@jetersen jetersen merged commit 836e045 into release-drafter:master Mar 3, 2022
@jetersen
Copy link
Member

jetersen commented Mar 3, 2022

@blast-hardcheese Thanks for your contribution! 👏

@jetersen jetersen added the type: feature New feature or request label Mar 3, 2022
@jetersen
Copy link
Member

jetersen commented Mar 3, 2022

@blast-hardcheese We forgot to regenerate the schema.json 😄

yarn generate-schema should do the trick 😅

@blast-hardcheese blast-hardcheese deleted the include-paths branch March 3, 2022 20:52
@blast-hardcheese
Copy link
Contributor Author

@jetersen Sorry about that! #1094 is up

@Kledal
Copy link

Kledal commented Mar 4, 2022

@blast-hardcheese thank you so much for this 👍

@blast-hardcheese
Copy link
Contributor Author

@Kledal I didn't realize others were waiting for this functionality too! Great to hear 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants