Skip to content

Conversation

@paul121
Copy link

@paul121 paul121 commented Nov 5, 2022

Description

Set options.uri using an environment variable instead of drush.yml config.

Related Issue

Please drop a link to the issue here: #127

Motivation and Context

  • Makes the template simpler
  • Makes it easier to provide a custom drush.yml config

See my comment in #127:

An added benefit of this approach is that it becomes much easier to use this template with your own .drush/drush.yml config file. As it stands you would need to update platformsh_generate_drush_yml.php to include additional values or run a subsequent command that amends the existing .drush/drush.yml. It would be much easier if this template didn't create the .drush/drush.yml in the first place.

How Has This Been Tested?

Please describe in detail how you tested your changes. Include details of your testing environment, and the tests you ran to see how your change affects other areas of the code, etc.

I applied this change to a development site on platformsh. Executing platform ssh drush -- status --fields uri yields a correctly configured site URI. UPDATE: I've also tested this on environment branches of the main/default project branch (eg: testing/staging) and made some followup to make this work there.

Screenshots (if appropriate):

Types of changes

What types of changes does your code introduce? Put an x in all the boxes that apply:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Maybe a breaking change? (fix or feature that would cause existing functionality to change)

Checklist:

Go over all the following list, and put an x in all the boxes that apply. If you're unsure about what any of these mean, don't hesitate to ask. We're here to help!

@rfay
Copy link
Contributor

rfay commented Nov 29, 2022

IMO DRUSH_OPTIONS_URI is usually a friendlier technique, but it may be funky in multisite situation. DDEV uses DRUSH_OPTIONS_URI.

.environment Outdated
fi

# Set the default site URL for drush.
export DRUSH_OPTIONS_URI=$(echo $PLATFORM_ROUTES | base64 --decode | jq -r 'map(select(.primary == true)) | first | .production_url')
Copy link

Choose a reason for hiding this comment

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

I have a couple questions:

  1. is production_url always set, and if not, doesn't that mean that DRUSH_OPTIONS_URI will be an empty string rather than undefined... and I wonder how drush would play with that environment variable being set but empty.

  2. How would this affect environments that are not main/master? Those URLs will most likely have no resemblance to the production url, and therefore would not allow drush to derive which multi-site to load.

Copy link
Author

@paul121 paul121 Dec 2, 2022

Choose a reason for hiding this comment

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

Thanks @apotek . After looking closer I think you are correct that I shouldn't be using the production_url... the production_url does seem to always be present, but is always the URL of the "default" branch.

For example, here is the $PLATFORM_ROUTES environment on a test branch. See that production_url is for the main (default) branch:

[email protected]:~$ echo $PLATFORM_ROUTES | base64 --decode | jq
{
  "https://test-t6dnbai-ev7iougo4jhve.us-2.platformsh.site/": {
    "primary": true,
    "id": null,
    "production_url": "https://main-bvxea6i-ev7iougo4jhve.us-2.platformsh.site/",
    "attributes": {},
    "type": "upstream",
    "upstream": "app",
    "original_url": "https://{default}/"
  },
  "https://www.test-t6dnbai-ev7iougo4jhve.us-2.platformsh.site/": {
    "primary": false,
    "id": null,
    "production_url": "https://www.main-bvxea6i-ev7iougo4jhve.us-2.platformsh.site/",
    "attributes": {},
    "type": "redirect",
    "to": "https://test-t6dnbai-ev7iougo4jhve.us-2.platformsh.site/",
    "original_url": "https://www.{default}/"
  },
  "http://test-t6dnbai-ev7iougo4jhve.us-2.platformsh.site/": {
    "original_url": "http://{default}/",
    "id": null,
    "primary": false,
    "type": "redirect",
    "to": "https://test-t6dnbai-ev7iougo4jhve.us-2.platformsh.site/",
    "production_url": "http://main-bvxea6i-ev7iougo4jhve.us-2.platformsh.site/"
  },
  "http://www.test-t6dnbai-ev7iougo4jhve.us-2.platformsh.site/": {
    "original_url": "http://www.{default}/",
    "id": null,
    "primary": false,
    "type": "redirect",
    "to": "https://www.test-t6dnbai-ev7iougo4jhve.us-2.platformsh.site/",
    "production_url": "http://www.main-bvxea6i-ev7iougo4jhve.us-2.platformsh.site/"
  }
}

This prompted me to look at how the platform config-reader library builds these routes. It seems we should be looking for all type = upstream routes. This is well documented in the source here! :-) https://github.com/platformsh/config-reader-php/blob/09982f28b0831401dc62a34fa3edceda76efaf19/src/Config.php#L325-L348

This makes the JQ a bit more complex because the URLs we want to target are JSON object keys, but I can replicate the same logic with this:

[email protected]:~$ echo $PLATFORM_ROUTES | base64 --decode | jq -r 'to_entries | map(select(.value.type == "upstream")) | sort_by(.value.primary, .key) | reverse | [.[].key] | first'
https://test-t6dnbai-ev7iougo4jhve.us-2.platformsh.site/

Copy link
Author

Choose a reason for hiding this comment

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

I've added a commit to implement this logic

Copy link

Choose a reason for hiding this comment

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

@paul121 Impressive sleuthing, and also excellent documentation of your thinking and discovery process. I think you found a great path forward.

@paul121
Copy link
Author

paul121 commented Dec 2, 2022

@rfay and @apotek thanks for the input. I agree that this may be more complicated for multisite, but there is a separate template for that: https://github.com/platformsh-templates/drupal9-multisite

FWIW that template seems to use the same logic that this template currently uses. And after addressing @apotek's comments I believe what I have implemented should behave the same as the current logic of this template (select upstream routes, sorted by primary and https)

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.

3 participants