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

Pass variables to replace tokens in query #809

Merged
merged 7 commits into from
Jun 14, 2018

Conversation

rafatower
Copy link
Contributor

This PR fixes #679. It comprises the following changes:

  • the patch in CartoDB@5182b71 cherry-picked on master
  • an end-to-end test for it
  • the dependency mapnik-vector-tile updated to current master commit (to be changed).

Some remarks:

  • As the change is built on Pass variables to replace tokens in query mapbox/mapnik-vector-tile#248 and takes advantage of the optional second argument of the vector tile processor constructor, updating to a newer version of mapnik-vector-tile is required, otherwise it will fail at build time. @springmeyer do you plan to release a 1.4.1 or 1.5.0 version anytime soon?
  • The test depends on having postgres+postgis installed. Otherwise it is skipped. I'd like to add it to CI builds, if possible.

Comments & suggestions welcomed.

@springmeyer
Copy link
Member

Noticed this is failing currently due to unrelated regression to ndoe v0.10.x support. Please rebase from master once #810 is merged.

@springmeyer do you plan to release a 1.4.1 or 1.5.0 version anytime soon?

mapnik-vector-tile@1.5.0 is now published.

The test depends on having postgres+postgis installed. Otherwise it is skipped. I'd like to add it to CI builds, if possible.

👍 And ideally on OSX as well as Travis. As you know postgres/postgis is a challenging install. I would prefer not to depend on third-party launchpad PPAs that change over time and may break unpredictably. For this reason I would prefer postgres/postgis is installed from mason, which I can update if needed to keep node-mapnik tests passing against different versions. Want to have a look at https://github.com/springmeyer/postgis-mason-install for an example of how to install this on travis? Or provide an alternative strategy?

@rafatower
Copy link
Contributor Author

rafatower commented Sep 13, 2017

I rebased from master, updated to mapnik-vector-tile@1.5.0 (thanks for the prompt release BTW), added the scripts to install postgis from mason and integrated them with travis build. Hope this is now ready to go. Thanks!

Copy link
Member

@springmeyer springmeyer left a comment

Choose a reason for hiding this comment

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

Looks good.


it('passes variables to replace tokens in query', function(done) {
if (!hasPostgisAvailable) {
this.skip('postgis not availabel');
Copy link
Member

Choose a reason for hiding this comment

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

Minor spelling fix needed: availabel -> available.

@springmeyer
Copy link
Member

@rafatower thanks for putting together the testing quickly. Looks great. I am headed on vacation for a week, but I will plan to test this myself and merge when back.

@springmeyer springmeyer added this to the v3.6.3 milestone Sep 14, 2017
@springmeyer springmeyer mentioned this pull request Sep 14, 2017
3 tasks
@springmeyer
Copy link
Member

springmeyer commented Jun 5, 2018

I will plan to test this myself and merge when back.

Sorry I've not gotten to this. I've become too busy with other responsibilities to dive into this code with the attention I feel is needed. This is a very cool feature, but needs testing to ensure it does not break anything in an unintended way. Is there anyone else reading this could provide:

  • a second set of eyes?
  • test the feature in their downstream application to ensure it works?
  • help ensure all the tests are passing on travis with this PR?

@springmeyer
Copy link
Member

Going to merge this into master without testing further. This is a rare exception for me, but the change is primarily tests so this should be safe. Please ping here if any hits any problems due to this change and we'll investigate.

@springmeyer springmeyer merged commit 429ae0a into mapnik:master Jun 14, 2018
@springmeyer springmeyer modified the milestones: v3.6.4, v3.7.3 Jun 18, 2018
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.

tokens in Mapnik postgis sql sub-selects not replaced using options.variables for VectorTiles
2 participants