-
-
Notifications
You must be signed in to change notification settings - Fork 62
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
fix(pullrequests) Do not mention a pull request in dry run when there are no changes #496
Conversation
@@ -70,7 +70,7 @@ func New(config *Config, sourceControlManager *scm.Scm) (PullRequest, error) { | |||
|
|||
p := PullRequest{ | |||
Title: config.Title, | |||
Config: config, | |||
Config: *config, |
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'm wondering how this change could help?
The type passed to the object was already a *Config
(e.g. a pointer) so the calling method on the PullRequest
were already able to change its pointer reference.
Did I miss something?
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.
That was my understanding as well but it doesn't seem to work. I think that part would benefit from so refactoring. But I first needs to master the debugger :p
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.
Given this consideration + the fact that the updatecli compiled from this PR stills creates PR with the litteral {{ source ...}}
, do you mind removing this commit, so we can proceed on the rest of the code changes (eg fixing #484) ?
Many thanks for taking time for this!
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.
For info, since this is only a "bugfix", I'm rebasing this PR to update it + remove the "PR title templating" tentative fix that would be a subsequent PR, so we can merge the initial fix.
@olblak do you mind opening an issue describing this thought process and pointing some code elements? That would allow us to discuss the subject, reach a consensus and then implement it in async :) |
Yep, yep, which is why I mentioned it in "Potential Improvement" section |
Signed-off-by: Olblak <me@olblak.com>
An expected pull request is mentioned in dry run logs even if there is no change
Fix #484
Fix #440
In the current state, this PR fix two bugs which by the way could be split into two Pull requests :)
Bump app to {{ source "source" }}
Test
To test this pull request, you can run the following commands:
Additional Information
Tradeoff
Potential improvement
While working on this PR, I realize that we have confusion about what a pull request title should be. The title should be configured in the spec level and not at the pull request level.
or
At the moment the latter example, is implement in the code in a way that it's totally useless.
I also realize that the way to generate a pullrequest title is overly complicated and could drastically simplified by just having the two following cases
1 A title is specified in the spec, then we use that value
2. No title specified so we let GitHub should, which will be the first commit title