-
Notifications
You must be signed in to change notification settings - Fork 15
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
improve code style #6
Conversation
3d9ce18
to
173aaa1
Compare
v = ENV['to'] | ||
require 'github_changelog_generator/task' | ||
GitHubChangelogGenerator::RakeTask.new :changelog do |config| | ||
config.future_release = v |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chris-rock suggestion: add config.user
and config.project
, this would allow the changelog generator to work with forks
Similar to https://github.com/dev-sec/chef-ssh-hardening/blob/master/Rakefile#L61-L62
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@artem-sidorenko But that is picked up from the git remote configuration. Do we really need that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chris-rock if your origin
remote is the main repositoriy - it works fine,
if your origin
remote is the fork - changelog generator does not find any issues/pr information
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use environment variables to overwrite the values? Would really like to make this optional input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chris-rock why? This both things are used by the log generator to get the information about the issues/PRs when accessing the project via GitHub API. Issues/PRs are always in the base repo within dev-sec organization.
If you do not want this things, just ignore it. The point is, if somebody works on the forks and not in the base repo within dev-sec org (like myself) and wants to update the Changelog - it does not work. You have either to change the remote origin
to the base repo within dev-sec organization or set this options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reason: I was thinking to extract the rake task to a small lib that we use for all projects. But you're right, we can do that in a second iteration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll go with your proposed solution until we have one that is easier to manage across all projects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chris-rock cool, +1 for a lib/gem :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated as discusseed
Signed-off-by: Christoph Hartmann <chris@lollyrock.com>
173aaa1
to
34f0b14
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @chris-rock
adds: