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

Use parent pom 4.77 #219

Merged
merged 7 commits into from
Jan 21, 2024
Merged

Use parent pom 4.77 #219

merged 7 commits into from
Jan 21, 2024

Conversation

shivajee98
Copy link
Contributor

@shivajee98 shivajee98 commented Jan 20, 2024

mvn versions:update-parent
Added null check in CopyArtifact.java and handle IllegalArgumentException

Testing Details:

  • Automated tests executed successfully.
  • Cloud build performed using mvn clean install on Codesapce

Submitter checklist

Edit tasklist title
Beta Give feedback Tasklist Submitter checklist, more options

Delete tasklist

Delete tasklist block?
Are you sure? All relationships in this tasklist will be removed.
  1. Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
    Options
  2. Ensure that the pull request title represents the desired changelog entry
    Options
  3. Please describe what you did
    Options

@shivajee98 shivajee98 requested a review from a team as a code owner January 20, 2024 03:44
Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

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

Please remove the pom.xml.versionsBackup file. It is not needed and will distract others.

@shivajee98
Copy link
Contributor Author

Please remove the pom.xml.versionsBackup file. It is not needed and will distract others.

ok

@MarkEWaite
Copy link
Contributor

Please retain the pull request template, including the Testing Done heading. Below the Testing Done heading, describe how you verified that the change is correct.

The pull request template means it when it says:

Testing done

Provide a clear description of how this change was tested.
At minimum this should include proof that a computer has executed the changed lines.
Ideally this should include an automated test or an explanation as to why this change has no tests.
Note that automated test coverage is less than complete, so a successful PR build does not necessarily imply that a computer has executed the changed lines.
If automated test coverage does not exist for the lines you are changing, you must describe the scenario(s) in which you manually tested the change.
For frontend changes, include screenshots of the relevant page(s) before and after the change.
For refactoring and code cleanup changes, exercise the code before and after the change and verify the behavior remains the same.

@MarkEWaite MarkEWaite changed the title pom.xml update Use parent pom 4.77 Jan 20, 2024
Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

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

@shivajee98 I told you in the earlier review of the pull request to split the pull request into multiple pull requests. You've again combined multiple changes into a single pull request. Please don't do that. It makes the change more difficult to review and makes the maintainer less likely to accept the change.

@shivajee98
Copy link
Contributor Author

@shivajee98 I told you in the earlier review of the pull request to split the pull request into multiple pull requests. You've again combined multiple changes into a single pull request. Please don't do that. It makes the change more difficult to review and makes the maintainer less likely to accept the change.

While code was going through the checks then SpotBugs identified issue of null exception, so i made the change here
I am trying to clear that

@MarkEWaite
Copy link
Contributor

While code was going through the checks then SpotBugs identified issue of null exception, so i made the change here I am trying to clear that

That makes sense. Thanks for the clarification.

Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

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

Please remove the IDE settings file .vscode/settings.json from the pull request. That setting is specific to your IDE and is unrelated to the topic of the pull request.

@MarkEWaite
Copy link
Contributor

Please add a "Testing done" section to the pull request description and describe the testing you used to confirm this change is correct and complete.

@shivajee98
Copy link
Contributor Author

shivajee98 commented Jan 20, 2024

Please add a "Testing done" section to the pull request description and describe the testing you used to confirm this change is correct and complete.

Ok sure
I really thank you for your patience, I learned a lot from this pull request.

@MarkEWaite
Copy link
Contributor

I made some style changes in 02e1fb3 because I prefer to define the variable near its use so that it is clear that the variable i is only being used in that branch of the conditional.

I made some spacing changes in 02e1fb3 because I like to keep the number of changes small between versions.

I did those myself rather than bothering you with them because they are purely questions of style and this plugin does not have a contributing guide that offers an opinion on those questions of style.

Are those changes acceptable to you @shivajee98 ? If so, then once the CI job passes, I believe this is ready to be squash merged.

Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

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

Thanks.

Since the previous behavior with a null projectName would have been to throw a null pointer exception, I think it is OK to throw an illegal argument exception.

@MarkEWaite MarkEWaite merged commit 25d996b into jenkinsci:master Jan 21, 2024
16 checks passed
@shivajee98 shivajee98 deleted the updates branch January 21, 2024 21:03
@MarkEWaite MarkEWaite added the dependencies Pull requests that update a dependency file label May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
2 participants