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

Create a new vectortile role #686

Merged
merged 0 commits into from
Oct 8, 2024
Merged

Conversation

pnorman
Copy link
Collaborator

@pnorman pnorman commented Aug 8, 2024

This role is for tilekiln vector tile servers and it currently sets up and starts tilekiln and has the scripts necessary to import data.

It's getting long enough that I'd like a review

property :owner, :kind_of => String, :required => [:create]
property :permissions, :kind_of => Hash, :default => {}

action :create do
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not happy with the complexity of this. Schemas are per-DB and created by chef with access permissions. This is different than other resources. Tablespaces are clusterwide and created with chef while tables are per-db and not created by chef.

@pnorman pnorman marked this pull request as ready for review August 9, 2024 18:34
@pnorman
Copy link
Collaborator Author

pnorman commented Aug 9, 2024

ping @Firefishy

@tomhughes tomhughes self-assigned this Aug 9, 2024
@tomhughes
Copy link
Member

I'm probably better placed to review this... I'll try and get to it over the weekend.

cookbooks/vectortile/README.md Outdated Show resolved Hide resolved
cookbooks/vectortile/templates/default/import-planet.erb Outdated Show resolved Hide resolved
cookbooks/vectortile/templates/default/import-planet.erb Outdated Show resolved Hide resolved
cookbooks/vectortile/recipes/default.rb Outdated Show resolved Hide resolved
cookbooks/vectortile/recipes/default.rb Outdated Show resolved Hide resolved
cookbooks/vectortile/recipes/default.rb Outdated Show resolved Hide resolved
cookbooks/vectortile/recipes/default.rb Outdated Show resolved Hide resolved
roles/vectortile.rb Outdated Show resolved Hide resolved
@pnorman
Copy link
Collaborator Author

pnorman commented Aug 19, 2024

I believe all issues have been fixed.

@pnorman
Copy link
Collaborator Author

pnorman commented Aug 19, 2024

Is it better to merge this in large chunks like this one, or wait till a machine is assigned and then merge it and assign the machine the appropriate role?

@tomhughes
Copy link
Member

Is it better to merge this in large chunks like this one, or wait till a machine is assigned and then merge it and assign the machine the appropriate role?

I don't mind when it's merged but I'd rather not have it amended once it's been reviewed - better to merge it and make changes in additional PRs as it's easier to review additional changes than have to re-review a big change like this because it's been updated.

@pnorman pnorman force-pushed the vectortile branch 3 times, most recently from c265a37 to 28a07fc Compare September 24, 2024 17:16
@pnorman pnorman requested a review from tomhughes September 24, 2024 17:42
@pnorman
Copy link
Collaborator Author

pnorman commented Sep 24, 2024

28a07fc is ready for review.

Copy link
Member

@tomhughes tomhughes left a comment

Choose a reason for hiding this comment

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

Can we just squash the commits together please - there doesn't seem to be any sort of structure to them other than the order in which they were made so it might as well be a single commit.

cookbooks/tile/attributes/default.rb Outdated Show resolved Hide resolved
@pnorman
Copy link
Collaborator Author

pnorman commented Oct 2, 2024

I think everything is good here. I've rebased on master and I'm successfully keeping the DB up to date and generating vector tiles

Copy link
Member

@tomhughes tomhughes left a comment

Choose a reason for hiding this comment

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

One thing to bear in mind is that we need to add the new users to the private repo before we merge this.

Disallow: /20/
Disallow: /21/
Disallow: /22/
Disallow: /23/
Copy link
Member

Choose a reason for hiding this comment

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

Is this right for vector tiles?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Zoom levels 20+ don't exist for our raster or vector tiles. I assumed they were added to robots.txt because people were requesting them, and the same could happen with vector tiles.

cookbooks/vectortile/recipes/default.rb Outdated Show resolved Hide resolved
cookbooks/vectortile/recipes/default.rb Outdated Show resolved Hide resolved
cookbooks/vectortile/recipes/default.rb Outdated Show resolved Hide resolved
@pnorman pnorman requested a review from tomhughes October 5, 2024 20:35
@pnorman
Copy link
Collaborator Author

pnorman commented Oct 5, 2024

The test failures are in db-base, master, and slave and seem unrelated to this PR.

Copy link
Member

@tomhughes tomhughes 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 is good enough to merge now...

@tomhughes tomhughes merged commit 3d9c577 into openstreetmap:master Oct 8, 2024
202 checks passed
@pnorman pnorman deleted the vectortile branch October 9, 2024 00:23
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