-
Notifications
You must be signed in to change notification settings - Fork 39
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
(#89) Add ability to populate release notes with information about contributors #541
base: develop
Are you sure you want to change the base?
Conversation
@gep13 please review at your convenience but do not merge yet. I just rebased your recent changes and I am now investigating how to implement my new feature in GitLat. UPDATE: I converted to draft to make sure this PR is not merged. |
2c7dd2c
to
9fdf779
Compare
I solved all the remaining issues. This logic is compatible with multiple issues/PRs being associated with a given issue/PRs and also the GetLinkedIssues method has been implemented for GitLab. |
@gep13 in case I wasn't clear: this PR is ready for review. |
In case anybody is interested to try this new feature, you can replace
in your cake script and you'll be able to generate release notes similar to this: https://github.com/Jericho/ZoomNet/releases/tag/0.71.0 |
@Jericho please accept my apologies for the radio silence on this PR! Simply put, I haven't had any time to even think about looking at this. I am hoping to review this in the coming days, and will provide some feedback (if there is any). Thanks again for your help with this! |
No apologies necessary. I'm just glad to be contributing. |
1b77d08
to
28df4ed
Compare
@Jericho apologies again! I haven't had a chance to look at this yet, and looks like I am going to have to do an immediate release to fix the issue with creating releases as a result of a required change upstream to Octokit. |
…uthor in the release notes
…st one contributor
…were included in release notes
…o be printed in the resulting markdown text
Just tried using the |
Please set a break point on line 66 in Do you get the same? |
And while we're on the topic of templates, double-check the presence of |
Doh! I have figured it out... I suddenly remembered about the ![]() You have to right click on that, and then click I don't know exactly when this is run as part of the normal build process, but it might also explain why you found you had to clean the files in order to get things to work properly. @AdmiringWorm might be able to provide some insight into when this part of the tooling works. Thinking about it, I "think" it runs as part of the Cake build, but I thought it ran inside Visual Studio as well. Now that I have this working, I should be able to take this for a proper trial. |
It is indeed run as part of the Cake build, it is a separate task called |
This is necessary because I merged the develop branch earlier today where the reference to NUnit's NuGet package was upgraded to version 4.x. If I understand the situation correctly, the 'legacy' assertion syntax was supported until version 3.x and was removed in 4.x. That's why my unit tests with the old syntax worked until I rebased my branch earlier today.
@Jericho 👋 apologies again for not coming back to you sooner about this one! I would try to give an excuse, but I really don't have one, so, moving on... A couple of questions/observations...
![]() Issue 113 was indeed raised by Geoffrey, however, there is a linked Pull Request that was raised by me. Shouldn't the description for this issue include the information about the link to the PR?
I haven't really got a strong opinion either way. If we go down the route of using a Configuration value to include/exclude the Contributor information, then we could control when this is instantiated? Or, as you say, perhaps split this out into a separate provider. That might make things a little more complicated in the long run though.
Again.. I am loving this PR, and the functionality that this provides! Thank you so much for putting the work in so far to get this to where it is! |
Regarding GitLab, I have just tested it on a repository that has an issue with a linked MR, but the generated release notes doesn't mention the associated MR. The |
Sure, no problem. I can add a Boolean property to the
Please make yourself comfortable, answering this seemingly simple question might take a while. Here goes my attempt to explain, based on my (potentially flawed) understanding. GitHub's API does not have a way to retrieve the list of issues/PRs linked to a given issue. The only workaround I was able to find is to use GitHub's graphql API. But even this API does not allow you to easily retrieve the linked issues/PRs. You have to retrieve so called "timeline events" of specific types (in our case we want "connected" and "disconnected"), sort them in chronological order and then we can derive which issues and PRs have been "connected" to the desired issue without having been disconnected. Here's a fictitious example: let's say you link a PR with the issue, realize you made a mistake and unlink this PR and finally you correct your mistake by linking another PR. In this fictitious scenario, the graphQL API would return the following timeline items:
In this example, you can see that PR 222 is initially associated to the desired issue but subsequently unlinked. and finally, PR 333 is linked and it's not unlinked. Based on these timeline events we can conclude that PR 333 is the one PR associated to the desired issue. I hope you're following me so far. FYI: this logic is similar to what is described in this StackOverflow comment. ok, so now that we have the theory out of the way, let's try to investigate specifically what is going on with issue 113 in the GitReleaseManager repo. Here's the tool I use to debug the graphql query and here's the query that you can copy and paste in this tool: {
repository(name: "GitReleaseManager", owner: "GitTools") {
issue(number: 113) {
timelineItems(first: 50, itemTypes: [CONNECTED_EVENT, DISCONNECTED_EVENT]) {
nodes {
__typename
... on ConnectedEvent {
createdAt
id
source {
__typename
... on Issue {
number
}
... on PullRequest {
number
}
}
subject {
__typename
... on Issue {
number
}
... on PullRequest {
number
}
}
}
... on DisconnectedEvent {
createdAt
id
source {
__typename
... on Issue {
number
}
... on PullRequest {
number
author {
avatarUrl
resourcePath
}
}
}
subject {
__typename
... on Issue {
number
}
... on PullRequest {
number
}
}
}
}
}
}
}
} After you click the "Run" button, you will see this result: As you can see in the result, there are no timeline events!!!! Not a single one!!!! Allow me to repeat what I just said, for dramatic effect: not a single one! In other words: GitHub is telling us that this issue was never connected to any other issue or PR. This does not make sense. You and I know that the issue is indeed linked to a PR, therefore we expect at the very least one "connect" timeline event. But the reality is that the API returns zero records. The ultimate question is: why is GitHub's graphQL Api returning data that seems incorrect or incomplete. I do not know with certainty the answer to this question but I have a theory (emphasis on "theory"): if you go back the the graphQL query that I presented earlier and that you copied/pasted into the GraphQL explorer, you'll notice that I added a "filter" when querying timeline items, like so: Anyway, I'm sure I have bored you to death with all this information. The TL;DR is that I suspect there's another type of event that in some scenarios, such as the one for issue number 113, represents the association of two records. I have no clue what if this theory is true and if it is, what would be the type of event in question. I have been using a beta copy of GRM to generate my release notes for at least 6 months and never noticed this problem. So I'm wondering if this issue you found is an edge case or if it's common.
In other words, leave
Separating it in a distinct provider would have been my preferred method but GitLab doesn't have this concept of a GraphQL client. So I was at a loss to design a way that works for both Git environments.
Yes indeed, much much much simpler. |
Woot! 😄
No, this wasn't exactly what I was referring to. Rather here at this location: https://github.com/GitTools/GitReleaseManager/tree/develop/src/GitReleaseManager.Core/Templates We create a new folder, perhaps named Does that make sense? This idea came from @AdmiringWorm, so if I haven't made this clear, perhaps he can help to elaborate.
Hmm, that is a fair point. What about adding an overload to the GitHubProvider, to pass in an instance of the GraphQL Client, and then in the Program.cs, based on when the contributors config value has been set, either call the new constructor with an instance of the client, or call the other one? That way, it is only created when needed. Just an idea, haven't dug into this too much, so I could be missing something here.
Just to level set here... Do you have an example of a GitLab issue/PR pair that works with the current code that you have in place? How exactly did you create the association between the two? The repository I was using for testing isn't public, so I can't share, but within the issue body, there is this section: ![]() Do you have that in the issue that you have created? Or do you have another section? If so, I would be curious to know how you have associated the two together, as I could be doing something wrong. |
…inct template called "contributors".
…ct that we want contributors to be included in the release notes or not.
…orresponds to a certain path. If the node is not present in the doc, fallback to a second path. If this second path is not present, fallback to a third path and so on.
….IndexOf' where the result is used to check for the presence/absence of a substring can be replaced by 'string.Contains'
…itLeb provider
This new config option will be available in GRM 0.19.0 (when PR GitTools#541 is merged).
Changes since last review:
Ready for another review |
Properly attribute each issue / PR which is part of a release.
Resolves #89