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

Duplicated/Incorrect PRs listed #221

Closed
whyman opened this issue May 27, 2019 · 11 comments · Fixed by #224 or #251
Closed

Duplicated/Incorrect PRs listed #221

whyman opened this issue May 27, 2019 · 11 comments · Fixed by #224 or #251

Comments

@whyman
Copy link

whyman commented May 27, 2019

Hi,

I am getting some random very old PRs linked, or incorrect IDs or something strange.

As you can see below "Fixture Upgrade" is duplicated and the 2nd one links to a random old PR.

Project is OSS: here is the config: https://github.com/gerbera/gerbera/blob/master/.github/release-drafter.yml

Please help!


image

@jetersen
Copy link
Member

@TimonVS I believe the GraphQL is doing something unexpected

@TimonVS
Copy link
Member

TimonVS commented May 27, 2019

I think I see what's going on here, the duplicate PRs are coming from forks. I will create a PR to filter those out :)
Thanks for reporting!

@TimonVS
Copy link
Member

TimonVS commented Jun 24, 2019

I'm reopening this issue because the fix I created turned out to be wrong. I will work on a definitive fix.

@TimonVS TimonVS reopened this Jun 24, 2019
@whyman
Copy link
Author

whyman commented Jul 1, 2019

Any news?

@TimonVS
Copy link
Member

TimonVS commented Jul 1, 2019

Hey @v00d00, I've thought out the following solution:

We change the query in lib/commits.js to the following:

...
                associatedPullRequests(first: 5) {
                  nodes {
+                   baseRepository {
+                     nameWithOwner
+                   }
                    url
                    title
                    number
                    author {
                      login
                    }
                    mergedAt
                    isCrossRepository
                    labels(first: 10) {
                      nodes {
                        name
                      }
                    }
...

Then we filter out all PRs where baseRepository.nameWithOwner is not equal to the name with owner of the repository we're generating the changelog for.

I think this should be the solution but I haven't tested it yet and there might still be some edge cases that I haven't thought of. I'm was also wondering if we should create an option for people to include PRs from forks in the changelog? Probably not though.

I've created a draft PR with the changes: #251. Though it doesn't include tests yet. If you're using the Actions version of Release Drafter you should be able to test it by changing the .github/main.workflow file:

workflow "Push" {
  on = "push"
  resolves = ["Draft Release"]
}

action "Draft Release" {
- uses = "toolmantim/release-drafter@v5.2.0"
+ uses = "TimonVS/release-drafter@fix/221"
  secrets = ["GITHUB_TOKEN"]
}

@whyman
Copy link
Author

whyman commented Jul 8, 2019

Unfortunately we use the "app" version.

@rnorth
Copy link
Contributor

rnorth commented Oct 26, 2019

We've just noticed this problem over in testcontainers/testcontainers-java, with a PR from our repo into someone's fork ending up in our draft release notes. This is the PR that caused it, triggering an entry in our release draft like this:

image

Strangely I've been unable to reproduce it myself today, though.

@toolmantim is it possible that this has been fixed in the last couple of weeks?

@toolmantim
Copy link
Collaborator

Strange! I haven't released an update in quite a while @rnorth

@jetersen
Copy link
Member

jetersen commented Oct 29, 2019

@toolmantim The logic the draft PR #251 @TimonVS should solve the problem

@jetersen
Copy link
Member

I'd like resume work adding a test.

@jetersen
Copy link
Member

@rnorth Thanks for pointing to a PR, made it easier to recreate a test.

I'll see if I can land a release to the app tomorrow or at least this week 😅
Few more PRs that would be nice to land.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants