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

Use npm packages for public/resources #1427

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

okimiko
Copy link
Contributor

@okimiko okimiko commented Dec 27, 2024

I wanted to add a dependency to my preview page and stumbled over the mirrored files and your discussion #1208

Here is my suggestion as "prepare" script including (the latest versions of)

  • leaflet
  • leaflet-hash
  • maplibre-gl
  • maplibre-gl-inspect

an updated Dockerfile (in general -> Using "&&" instead of ";" to chain commands) and especially with the added "copyfiles" regarding the new "prepare" step.

@okimiko okimiko marked this pull request as ready for review January 15, 2025 11:47
@acalcutt
Copy link
Collaborator

acalcutt commented Jan 16, 2025

The only thing that concerns me with this is dependabot automatically upgrading to major releases. I think we need to double check the dependabot settings because it automatically merged the last major maplibre-native and I didn't expect it would.

I know the next release of leaflet is a breaking change because the L. syntax is going away, although I am pretty sure the code we have that uses it doesn't actually do anything (since it deals with pixel ratio, which leaflet needs @2x in the (url) path...not like our preview page uses.)

I also know maplibre just experienced so pushback with their geometry type change that ended up getting revert. it seems to me some manual decision to change major releases would be good for this type of thing.

point being, i think this would be fine as long as we make sure dependabot is fixed so it doesn't automerge major releases.... but I still need to look at that.

@okimiko
Copy link
Contributor Author

okimiko commented Jan 16, 2025

Maybe this helps (untested, just of the docs):

version: 2
updates:
  - package-ecosystem: npm
    versioning-strategy: increase
    directory: '/'
    schedule:
      interval: daily
    ignore:
      - dependency-name: "leaflet"
        update-types: ["version-update:semver-major"]
      - dependency-name: "leaflet-hash"
        update-types: ["version-update:semver-major"]
      - dependency-name: "@mapbox/mapbox-gl-rtl-text"
        update-types: ["version-update:semver-major"]
      - dependency-name: "@maplibre/maplibre-gl-inspect"
        update-types: ["version-update:semver-major"]
      - dependency-name: "maplibre-gl"
        update-types: ["version-update:semver-major"]
    commit-message:
      prefix: fix
      prefix-development: chore
      include: scope
  - package-ecosystem: github-actions
    directory: '/'
    schedule:
      interval: weekly
    commit-message:
      prefix: fix
      prefix-development: chore
      include: scope

@acalcutt
Copy link
Collaborator

I think we actually might need to look at the automerger
https://github.com/maptiler/tileserver-gl/blob/master/.github/workflows/automerger.yml

I was looking at it a bit yesterday and we would still want dependabot to make a PR for major changes, but we might need to see if we can adjust the automerger so it doesn't automatically merge major releases

@acalcutt
Copy link
Collaborator

based on this it looks like we could set the target to minor
https://github.com/fastify/github-action-merge-dependabot?tab=readme-ov-file#specifying-target-for-development-and-production-packages-separately

@okimiko
Copy link
Contributor Author

okimiko commented Jan 16, 2025

Check. For automerger exists a target parameter, too:

A flag to only auto-merge updates based on Semantic Versioning.
Possible options are: major, premajor, minor, preminor, patch, prepatch, prerelease, any.

https://github.com/fastify/github-action-merge-dependabot?tab=readme-ov-file#inputs

@okimiko
Copy link
Contributor Author

okimiko commented Jan 16, 2025

From my perspective it does not make sense to handle development different than production: Major releases may cause problems in both cases. But feel free to change it, if you have a different opinion or experience.

@acalcutt
Copy link
Collaborator

acalcutt commented Jan 16, 2025

I agree that we should just target minor. and and major updates can be manually approved

I did just update the release workflow to allow pre-releases,, so in the future i think it will allow some level of testing. in the past this project didn't have pre-releases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants