-
Notifications
You must be signed in to change notification settings - Fork 20
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
Update PR title and body when relaunching a campaign #711
base: main
Are you sure you want to change the base?
Update PR title and body when relaunching a campaign #711
Conversation
Hi @jonesbusy Sir, I hope this message finds you well. I’ve successfully raised the PR, and all CI checks have passed. However, I haven’t added unit tests for the new changes yet in the PR. Should I proceed with adding the unit tests now? Thank you! |
Yes tests ensure that it work as expected. Thanks |
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.
Minor changes and tests missing
@@ -106,6 +106,19 @@ public void validate() { | |||
} | |||
} | |||
|
|||
public void updatePullRequestTitleAndBody(GHPullRequest pr, String newTitle, String newBody) throws IOException { |
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.
Shouldn't it be private?
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.
Surely Sir , I will updating it to private
LOG.info("Pull request already exists: {}", pr.getHtmlUrl()); | ||
updatePullRequestTitleAndBody(pr, prTitle, prBody); | ||
} else { | ||
LOG.warn("Existing pull request is null. Skipping update."); |
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.
Why a warning? Is just a normal behavior of no PR exist for a given recipe
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.
Thank you for the feedback. I used warn because I initially thought that having no existing pull request might be an unusual situation. However, I understand your point that it is a normal behavior when no PR exists for a given recipe.
I will update the code to use info instead of warn for this log message.
Optional<GHPullRequest> existingPR = checkIfPullRequestExists(plugin); | ||
if (existingPR.isPresent()) { | ||
GHPullRequest pr = existingPR.get(); | ||
if (pr != null) { |
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.
Why? It's an optional and we check it exists just before
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.
Thank you for pointing that out Sir. You're right, the null check is redundant since we are already checking if the Optional is present. I will remove the unnecessary null check and update the code accordingly.
I appreciate your feedback!
Okay Sir I will add and test it now ! |
Hi Sir , Thank you for the feedback. I have made the following changes as per your suggestions:
I will now proceed to add the unit tests for the new changes and update you here. Thank you for your guidance! |
...n-modernizer-core/src/main/java/io/jenkins/tools/pluginmodernizer/core/github/GHService.java
Outdated
Show resolved
Hide resolved
...n-modernizer-core/src/main/java/io/jenkins/tools/pluginmodernizer/core/github/GHService.java
Outdated
Show resolved
Hide resolved
...n-modernizer-core/src/main/java/io/jenkins/tools/pluginmodernizer/core/github/GHService.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Valentin Delaye <[email protected]>
Thanks Sir for the feedbacks I am applying your suggestion and also will update the branch. |
Yes but unit test must be implemented along of the changes. It's also a way to show that the feature is working. By reading the code it looks ok. But without tests or evidence (for example a screenshot of and end to end test) I don't know if it works as expected. So please implemented tests |
Thanks Sir for your help , I will now build the test and see if the changes works and also provide screenshots before finally updating you here . |
Open question. What happen if the result of a recipe change For example I expect jenkinsci/date-parameter-plugin#21 to be updated with 2.463.3 (not only the PR title or body if not this will not match the diff of the PR) @gounthar Did you saw a similar use case before? I'm wonder if we need to "force" push the branch in order to update the changes This definitely need to be tested before naïvely update the PR title and body but not update the PR content |
Hi Sir, Thank you for pointing this out. I understand the concern about ensuring the PR title, description, and content (code changes) remain consistent. I will test thoroughly to verify that the title and body updates match the actual PR content and no discrepancies occur; Thank you for your guidance! |
I have tested already but ran through some errors which I am trying to fix at the moment. |
Looking at git.reset().setMode(ResetCommand.ResetType.HARD).setRef(defaultBranch).call(); and List<PushResult> results = StreamSupport.stream(git.push().setForce(true) It looks is already covered |
Okay Sir. |
Improved one integration test to perform a second execution: #721 This should ensure that the new content is pushed |
Thanks Sir , much appreciated! |
Hello Sir , Sir I am repeatedly getting errors in my unit tests . |
|
I am not getting that these are errors from my end ,or it from original one . |
|
|
So I must be the way test are implemented? Maybe miss some mocks. You can check how others are implemented for GHService. I think one test that performed the update is enough. It's possible that one test for opening PR already exists. Did you base your test on this one? |
Fix #646
Description
-This pull request updates the functionality to ensure that the title and body of a pull request are updated when relaunching a campaign. This enhancement ensures that the pull request reflects the latest changes and feedback
Testing done
-Unit Tests done
-Will be adding the final test after original test issues get resolved
Summary of Changes
updatePullRequestTitleAndBody
method to update the title and body of an existing pull request.openPullRequest
method to check for existing pull requests and update their title and body if necessary.Impact on Existing Functionality
Submitter checklist
Additional Notes