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

Add ddev start for the host command #10

Merged

Conversation

stasadev
Copy link
Member

@stasadev stasadev commented Dec 7, 2023

The Issue

How This PR Solves The Issue

In the next release of DDEV, there will be no automatic project start when the host command is executed, so this PR adds the necessary ddev start here.

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

I think this approach will break people who get the addon with earlier ddev versions. Probably better to see if it's set before taking action, don't do anything if var not set. In "launch" and family we know it should be set but here we don't.

@stasadev stasadev force-pushed the 20231207_stasadev_ddev_start_host_command branch from 9e0ff78 to a21e861 Compare December 8, 2023 11:11
@stasadev
Copy link
Member Author

stasadev commented Dec 8, 2023

Yeah, right, didn't test it with a stable DDEV.

Fixed by setting the default value only when the variable is unset.
${DDEV_PROJECT_STATUS-running} https://stackoverflow.com/a/16753536

@stasadev stasadev requested a review from rfay December 8, 2023 11:15
Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

Thanks, this seems like a good technique, looks to me like syntax is wrong? At least I've only ever used ${VAR:-default}

@@ -5,6 +5,11 @@
## Usage: phpmyadmin
## Example: "ddev phpmyadmin"

if [ "${DDEV_PROJECT_STATUS-running}" != "running" ]; then
Copy link
Member

Choose a reason for hiding this comment

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

I like the technique, it's a good one. But it should be like this, right?

Suggested change
if [ "${DDEV_PROJECT_STATUS-running}" != "running" ]; then
if [ "${DDEV_PROJECT_STATUS:-running}" != "running" ]; then

Copy link
Member Author

Choose a reason for hiding this comment

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

No, this can give an unexpected result for the case when $DDEV_PROJECT_STATUS is defined but empty.

bad:

DDEV_PROJECT_STATUS="" && echo "status=${DDEV_PROJECT_STATUS:-running}"
status=running

good:

DDEV_PROJECT_STATUS="" && echo "status=${DDEV_PROJECT_STATUS-running}"
status=

Copy link
Member Author

Choose a reason for hiding this comment

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

POSIX: Parameter Expansion Examples: https://stackoverflow.com/a/16753536/8097891

Copy link
Member Author

Choose a reason for hiding this comment

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

I tested this behavior on macOS and it works the same way.
I'm going to merge ${DDEV_PROJECT_STATUS-running}" because it works exactly as it should.

@stasadev stasadev merged commit 9cc65e0 into ddev:main Dec 12, 2023
2 checks passed
@stasadev stasadev deleted the 20231207_stasadev_ddev_start_host_command branch December 12, 2023 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants