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

Don't abort on ddev release show changes when no changes are found #15932

Merged
merged 2 commits into from
Sep 29, 2023

Conversation

alopezz
Copy link
Contributor

@alopezz alopezz commented Sep 29, 2023

What does this PR do?

Stops ddev release show changes from aborting when no changes have been found, so that it retains behavior that other commands rely upon.

Motivation

AITS-247

Aborting causes the application to exit regardless of context, which is a problem when that breaks expectations of other commands that rely on invoking this command directly.

This was causing release make all to abort early as soon as an integration didn't have a change. This regression was introduced in #15378; we weren't aborting under that scenario before.

Additional Notes

Longer term, I think we should try to avoid invoking commands from within any sort of minimally complex logic, and be careful with where we call functions that can exit the interpreter completely. We can leave this as it is for now until we move this to the ddev package. There's also a couple of unused options that could be removed when we do that as well.

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • Changelog entries must be created for modifications to shipped code
  • Add the qa/skip-qa label if the PR doesn't need to be tested during QA.

@alopezz alopezz requested review from a team as code owners September 29, 2023 09:22
Aborting causes the application to exit regardless of context, which
is a problem when that breaks expectations of other commands that rely
on invoking this command directly.
@codecov
Copy link

codecov bot commented Sep 29, 2023

Codecov Report

Merging #15932 (e360bd7) into master (ac52e8b) will increase coverage by 0.10%.
The diff coverage is n/a.

Flag Coverage Δ
activemq ?
cassandra ?
datadog_checks_dev 82.51% <ø> (+0.07%) ⬆️
hive ?
hudi ?
ignite ?
jboss_wildfly ?
kafka ?
presto ?
solr ?
tomcat ?

Flags with carried forward coverage won't be shown. Click here to find out more.

@github-actions
Copy link

Test Results

    4 files      4 suites   4m 19s ⏱️
418 tests 418 ✔️   0 💤 0
936 runs  903 ✔️ 33 💤 0

Results for commit e360bd7.

@alopezz alopezz merged commit ede5af1 into master Sep 29, 2023
30 of 31 checks passed
@alopezz alopezz deleted the alopez/ddev/fix-changes branch September 29, 2023 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants