Skip to content
This repository was archived by the owner on Aug 29, 2020. It is now read-only.

Relax label checks #30

Merged
merged 9 commits into from
Oct 7, 2016
Merged

Relax label checks #30

merged 9 commits into from
Oct 7, 2016

Conversation

@andreasohlund
Copy link
Member

@andreasohlund andreasohlund commented Oct 6, 2016 •
edited
Loading

Since we're experimenting with a different lableling scheme for the core we're relaxing the requirements to always have a Type: lable. Such rules can be enforced with other means if needed.

Other changes

  • Enabled the tests to run on the buildserver
  • Made get milestones async to avoid deadlocking the tests
  • Update dependencies (thought there was a octokit bug)
  • Killed the "release manager" since that was my own bad idea from the past
  • Added all unlabeled issues/pulls into a Features/Improvements section
  • Cleaned up the readme
@andreasohlund
Copy link
Member Author

andreasohlund commented Oct 6, 2016

cc @Particular/nservicebus-maintainers
danielmarbach
danielmarbach suggested changes Oct 6, 2016
View reviewed changes
Copy link
Contributor

@danielmarbach danielmarbach left a comment

Choose a reason for hiding this comment

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

A few comments
src/App/Program.cs Outdated
{
Draft = false,

Copy link
Contributor

@danielmarbach danielmarbach Oct 6, 2016

Choose a reason for hiding this comment

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

Space not really needed
Copy link
Member Author

@andreasohlund andreasohlund Oct 6, 2016

Choose a reason for hiding this comment

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

fixed
src/App/app.config Outdated
@@ -0,0 +1,3 @@
<?xml version="1.0" encoding="utf-8"?>
Copy link
Contributor

@danielmarbach danielmarbach Oct 6, 2016

Choose a reason for hiding this comment

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

I thinl you can ditch this
Copy link
Member Author

@andreasohlund andreasohlund Oct 6, 2016

Choose a reason for hiding this comment

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

app.config removed
src/Compiler/ReleaseNotesBuilder.cs Outdated
public async Task<string> BuildReleaseNotes()
{
LoadMilestones();
milestones = await gitHubClient.GetMilestones();
Copy link
Contributor

@danielmarbach danielmarbach Oct 6, 2016

Choose a reason for hiding this comment

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

Do we need the field assignment? Would it be better to pass in the milestones into the methods that need it?
Copy link
Member Author

@andreasohlund andreasohlund Oct 6, 2016

Choose a reason for hiding this comment

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

Yea, didn't want to churn to much but I'll fix that ugly one
src/Compiler/ReleaseNotesBuilder.cs
milestones = await gitHubClient.GetMilestones();

GetTargetMilestone();
var issues = await GetIssues(targetMilestone);
Copy link
Contributor

@danielmarbach danielmarbach Oct 6, 2016

Choose a reason for hiding this comment

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

Above method GetTargegMilestone sounds like a side effect free method but does assign magically a field. Like above I would return the milestone. Then this method used mostly locals and we can ditch the fields
Copy link
Member Author

@andreasohlund andreasohlund Oct 6, 2016

Choose a reason for hiding this comment

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

Fields now ditched
timbussmann
timbussmann reviewed Oct 7, 2016
View reviewed changes
README.md
* All closed issues for a milestone will be included.
* All issues must have exactly one `Type: xxx` label. E.g. `Type: Bug`, `Type: Feature`, `Type: Refactoring`, etc. Only issues labelled `Type: Bug` and `Type: Feature `will be included in the release notes. Issues with other `Type: xxx` labels will be included in the milestone but excluded from the release notes.
* All closed issues/PR's for a milestone will be included.
* Issues/PR's with a label `Type: Bug` will be included in a `Bugs` section
Copy link

@timbussmann timbussmann Oct 7, 2016

Choose a reason for hiding this comment

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

what about the Bug label (without the type)?
Copy link
Member Author

@andreasohlund andreasohlund Oct 7, 2016

Choose a reason for hiding this comment

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

Will fix
src/Compiler/ReleaseNotesBuilder.cs
var commitsText = Format(numberOfCommits == 1 ? "{0} commit" : "{0} commits", numberOfCommits);

stringBuilder.AppendFormat(@"As part of this release we had [{0}]({1}) which resulted in [{2}]({3}) being closed.", commitsText, commitsLink, issuesText, targetMilestone.HtmlUrl());
stringBuilder.Append($"As part of this release we had [{commitsText}]({commitsLink}) which resulted in [{issuesText}]({targetMilestone.HtmlUrl()}) being closed.");
Copy link

@timbussmann timbussmann Oct 7, 2016

Choose a reason for hiding this comment

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

tbh I'm not a big fan of the commit numbers. Even for a single bugfix it results in at least 2 commits because of the additional merge commit. How about removing that part?
Copy link
Member Author

@andreasohlund andreasohlund Oct 7, 2016

Choose a reason for hiding this comment

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

Raise a separate pull, this one is already getting to big?
src/Compiler/ReleaseNotesBuilder.cs Outdated
Append(issues, "Feature", stringBuilder);
Append(issues, "Bug", stringBuilder);
var bugs = issues
.Where(issue => issue.Labels.Any(label => label.Name == "Type: Bug"))
Copy link

@timbussmann timbussmann Oct 7, 2016

Choose a reason for hiding this comment

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

how about Contains("Bug") ?
Copy link
Member Author

@andreasohlund andreasohlund Oct 7, 2016

Choose a reason for hiding this comment

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

Isn't that to wide, what if there is a label that contains those characters?

How about:

issue => issue.Labels.Any(label => label.Name == "Type: Bug" || label.Name == "Bug"
Copy link

@timbussmann timbussmann Oct 7, 2016

Choose a reason for hiding this comment

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

good with that either as of now (maybe some repo decided to have a Critical Bug or Type:Bug label which would break this impl. but good to skip that as of now.
src/Compiler/ReleaseNotesBuilder.cs Outdated
{
get { return "Type: "; }
}
public static string LabelPrefix => "Type: ";
Copy link

@timbussmann timbussmann Oct 7, 2016

Choose a reason for hiding this comment

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

this doesn't seem to be used in this class itself?
Copy link
Member Author

@andreasohlund andreasohlund Oct 7, 2016

Choose a reason for hiding this comment

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

Use by the tests but I think I can remove
@andreasohlund
Copy link
Member Author

andreasohlund commented Oct 7, 2016

@timbussmann all you comments should now be addressed
@danielmarbach danielmarbach merged commit 4a1c17e into master Oct 7, 2016
@danielmarbach danielmarbach deleted the relax-label-checks branch October 7, 2016 08:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

4 participants

@andreasohlund @danielmarbach @timbussmann