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

Add links to the referenced/fixed issues #59

Merged
merged 1 commit into from Jun 19, 2017
Merged

Add links to the referenced/fixed issues #59

merged 1 commit into from Jun 19, 2017

Conversation

akofink
Copy link

@akofink akofink commented Apr 6, 2017

as a comment on a new PR

Open to discussion!

@bbuckingham
Copy link
Member

bbuckingham commented Apr 7, 2017

@akofink, +1 for me. It would save me the time of formulating the link for every PR.... Thanks!

app.rb Outdated
@@ -78,6 +78,13 @@
end

if ENV['GITHUB_OAUTH_TOKEN']
if pull_request.new? && pull_request.issue_numbers.any?
Copy link

Choose a reason for hiding this comment

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

Please add this to the per-repo configuration.

Katello/hammer-cli-sam:
redmine: katello
Katello/katello:
pr_scanner: true
redmine: katello
redmine_required: true
link_to_redmine: true
Copy link
Member

Choose a reason for hiding this comment

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

You can add this to katello-installer and katello-packaging

@akofink
Copy link
Author

akofink commented Apr 17, 2017

This should be good to go.

@tbrisker
Copy link
Member

👍 I'd love to have this in foreman core as well tbh.

@akofink
Copy link
Author

akofink commented May 5, 2017

Updated to include foreman core.

@akofink
Copy link
Author

akofink commented May 24, 2017

Any updates on this?

@akofink
Copy link
Author

akofink commented Jun 12, 2017

Rebased. Waiting on reviewers.

message = issue_numbers.inject("\nIssues:") do |msg, issue_number|
msg + " [##{issue_number}](http://projects.theforeman.org/issues/#{issue_number})"
end
append_pull_request_body(message)
Copy link
Member

Choose a reason for hiding this comment

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

So this does not add a fresh comment? How does the output on a PR come out to look like if this is updating the issue? Are we altering the users first comment ?

Copy link
Author

Choose a reason for hiding this comment

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

Correct, it adds the issue links to the description of the PR on a new line. One big advantage is that no new notifications will be sent to subscribers on that PR/project. It looks like this:

The original description the user makes
Issues: #19023 #14611

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we should be editing other users comments and posts directly. While it does create an additional notification, this would be altering a users comment and making it seem as if the user posted that? If I do a merge instead of squash or rebase does this edit get put into the commit message for the merge?

Copy link
Author

Choose a reason for hiding this comment

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

This absolutely does not modify the commit message, only the pull request description on a new PR with issue numbers in the commit message.

@akofink
Copy link
Author

akofink commented Jun 15, 2017

Updated to add a comment instead of appending the description @ehelms

@ehelms ehelms merged commit dcb54c8 into theforeman:master Jun 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants