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.

Sign up for GitHub

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

Jump to bottom

JobLauncherApplicationRunner returns a success exit code even when no jobs have been run #36060

Closed
wants to merge 5 commits into from
Closed

JobLauncherApplicationRunner returns a success exit code even when no jobs have been run #36060

wants to merge 5 commits into from

Conversation

itsAkshayDubey
Copy link
Contributor

JobLauncherApplicationRunner returns a success exit code even when no jobs have been run.
fix: Rethrowing exception when NoSuchJobException is caught for non configured jobs.
Closes: #35940

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 24, 2023
@itsAkshayDubey itsAkshayDubey mentioned this pull request Jun 24, 2023
Closed
scottfrederick pushed a commit to scottfrederick/spring-boot that referenced this pull request Jun 29, 2023
@itsAkshayDubey @scottfrederick
Throw exception when a named batch job does not exist
1bb1925 
See spring-projectsgh-36060
scottfrederick added a commit to scottfrederick/spring-boot that referenced this pull request Jun 29, 2023
@scottfrederick
Polish "Throw exception when a named batch job does not exist"
bfa8996 
See spring-projectsgh-36060
scottfrederick
scottfrederick reviewed Jun 30, 2023
View reviewed changes
.../src/test/java/org/springframework/boot/autoconfigure/batch/BatchAutoConfigurationTests.java Outdated
.withPropertyValues("spring.batch.job.names:discreteLocalJob,nonConfiguredJob")
.run((context) -> {
assertThat(context).hasSingleBean(JobLauncher.class);
assertThrows(NoSuchJobException.class,()->{
Copy link
Contributor

Choose a reason for hiding this comment

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

The Spring Boot style rules don't allow JUnit assertThrows(). Please use the AssertJ assertThatIllegalArgumentException() or assertThatExceptionOfType() instead.

@scottfrederick
Copy link
Contributor

@itsAkshayDubey Thanks for the PR. As also suggested in the issue comments, it would be better to fail early if any job names are unknown instead of potentially processing some jobs and then failing. Starting in Spring Boot 3.0, some validation is done in a @PostConstruct method of JobLauncherApplicationRunner. Can you make changes to this PR so that this validation of job names is also done in a @PostConstruct method? That would also make it easier to forward-merge the changes to later release branches.

@itsAkshayDubey
Copy link
Contributor Author

Hi @scottfrederick ,

Added suggested changes, kindly have a look.

Thanks,
Akshay

@snicoll snicoll self-assigned this Jul 24, 2023
@snicoll snicoll added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Jul 24, 2023
@snicoll snicoll added this to the 2.7.15 milestone Jul 24, 2023
@snicoll snicoll mentioned this pull request Jul 24, 2023
Closed
snicoll pushed a commit that referenced this pull request Jul 24, 2023
@itsAkshayDubey @snicoll
Fail fast if job name does not exist
c38cd74 
See gh-36060
snicoll added a commit that referenced this pull request Jul 24, 2023
@snicoll
Polish "Fail fast if job name does not exist"
854c162 
See gh-36060
@snicoll snicoll closed this in 45f685f Jul 24, 2023
@snicoll
Copy link
Member

snicoll commented Jul 24, 2023

@itsAkshayDubey thank you for making your first contribution to Spring Boot.

@Netyyyy Netyyyy mentioned this pull request Aug 25, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scottfrederick scottfrederick scottfrederick left review comments

Assignees

@snicoll snicoll

Labels
type: bug A general bug
Projects
None yet
Milestone
2.7.15
Development

Successfully merging this pull request may close these issues.
4 participants
@itsAkshayDubey @scottfrederick @snicoll @spring-projects-issues