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

feat: Platform.sh specific app pull, fixes #5727 #5728

Merged
merged 5 commits into from Jan 26, 2024

Conversation

nir-riskified
Copy link
Contributor

@nir-riskified nir-riskified commented Jan 23, 2024

…an one app under your env.

The Issue

Today there is no option to choose an app when pulling data from platform.sh using ddev pull platform

How This PR Solves The Issue

Adding new env variable support PLATFORM_APP, when this variable exists we are adding --app flag

Manual Testing Instructions

Automated Testing Overview

Related Issue Link(s)

#5727

Release/Deployment Notes

@nir-riskified nir-riskified requested a review from a team as a code owner January 23, 2024 12:31
@nir-riskified nir-riskified changed the title Added support to pull data from specific APP in case you have more th… feat: Added support to pull data from specific APP in case you have more th… Jan 23, 2024
@nir-riskified nir-riskified changed the title feat: Added support to pull data from specific APP in case you have more th… feat: specific APP pull - for #5727 Jan 23, 2024
@nir-riskified nir-riskified changed the title feat: specific APP pull - for #5727 feat: Platform.sh specific APP pull - for #5727 Jan 23, 2024
Copy link
Member

@stasadev stasadev left a comment

Choose a reason for hiding this comment

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

I haven't actually tested platform integration with --app, but I suggest a more elegant way of writing the condition.

Used ${parameter:+word} from https://www.gnu.org/software/bash/manual/html_node/Shell-Parameter-Expansion.html

If parameter is null or unset, nothing is substituted, otherwise the expansion of word is substituted.

pkg/ddevapp/dotddev_assets/providers/platform.yaml Outdated Show resolved Hide resolved
pkg/ddevapp/dotddev_assets/providers/platform.yaml Outdated Show resolved Hide resolved
pkg/ddevapp/dotddev_assets/providers/platform.yaml Outdated Show resolved Hide resolved
pkg/ddevapp/dotddev_assets/providers/platform.yaml Outdated Show resolved Hide resolved
pkg/ddevapp/dotddev_assets/providers/platform.yaml Outdated Show resolved Hide resolved
pkg/ddevapp/dotddev_assets/providers/platform.yaml Outdated Show resolved Hide resolved
@stasadev
Copy link
Member

@nir-riskified,

I checked if all changed commands have --app option, everything looks fine.

You can apply my suggestions in one commit on the Files tab.

And please update the documentation file when you get a chance.

Thank you for the contribution!

nir-riskified and others added 2 commits January 24, 2024 10:52
Co-authored-by: Stanislav Zhuk <stasadev@gmail.com>
@nir-riskified nir-riskified requested a review from a team as a code owner January 24, 2024 09:02
@nir-riskified
Copy link
Contributor Author

Hey @stasadev I've applied your suggestions and updated the document file accordingly. Let me know if there's anything else. Thanks for your input!"

Copy link

github-actions bot commented Jan 24, 2024

Copy link
Member

@stasadev stasadev left a comment

Choose a reason for hiding this comment

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

@nir-riskified, I like your change in the docs with a new section for multiple apps, let's use it for information about the optional PLATFORM_APP instead of mixing it with other variables.

docs/content/users/providers/platform.md Outdated Show resolved Hide resolved
docs/content/users/providers/platform.md Outdated Show resolved Hide resolved
docs/content/users/providers/platform.md Outdated Show resolved Hide resolved
pkg/ddevapp/dotddev_assets/providers/platform.yaml Outdated Show resolved Hide resolved
@stasadev stasadev changed the title feat: Platform.sh specific APP pull - for #5727 feat: Platform.sh specific app pull, fixes #5727 Jan 24, 2024
Co-authored-by: Stanislav Zhuk <stasadev@gmail.com>
@nir-riskified
Copy link
Contributor Author

Updated! Thanks @stasadev

Copy link
Member

@stasadev stasadev left a comment

Choose a reason for hiding this comment

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

Added one more commit with indentation fix in docs, and renamed the header "Multiple apps" to "Managing Multiple Apps".

Let's see how the tests go.

@stasadev stasadev merged commit 80a490b into ddev:master Jan 26, 2024
19 checks passed
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.

None yet

2 participants