caksoylar / keymap-drawer

Visualize keymaps that use advanced features like hold-taps and combos, with automatic parsing
https://caksoylar.github.io/keymap-drawer
MIT License
730 stars 62 forks source link

Allow amending commit vs. creating one, add YAML #34

Closed minusfive closed 1 year ago

minusfive commented 1 year ago

@caksoylar how terrible of an idea is this?

This PR:

TODO:

Warning You should understand the implications of rewriting history if you amend a commit that has already been published. See rebasing.

You can see it working here https://github.com/minusfive/zmk-config/commits/main

Action YAML:

name: Draw ZMK keymaps
on:
  workflow_dispatch: # can be triggered manually
  push: # automatically run on changes to following paths
    paths:
      - "config/*.keymap"
      - "config/*.dtsi"
      - "keymap_drawer.config.yaml"

jobs:
  draw:
    uses: minusfive/keymap-drawer/.github/workflows/draw-zmk.yml@commit-options
    permissions:
      contents: write
    with:
      install_branch: "main" # branch to install keymap-drawer from
      keymap_patterns: "config/*.keymap" # path to the keymaps to parse
      config_path: "keymap_drawer.config.yaml" # config file, ignored if not exists
      output_folder: "img" # path to save produced SVGs
      # commit_message: "Draw: ${{ github.event.head_commit.message }}"
      ammend_commit: true # whether to ammend the commit or create a new one

A few things to note:

When you use the repository's GITHUB_TOKEN to perform tasks, events triggered by the GITHUB_TOKEN, with the exception of workflow_dispatch and repository_dispatch, will not create a new workflow run. This prevents you from accidentally creating recursive workflow runs.

caksoylar commented 1 year ago

Thanks! I think this is a good idea overall, as long as it is kept to an option like this (because it messes with the history).

A few notes on top of the review items above:

I tested simply allowing the user to set any commit / push options they'd like, but deemed it too dangerous, as you really have to get all the pieces right to avoid messing up the history (e.g. checkout depth, skip_fetch, etc). So figured better/safer to make it a simple boolean option

Makes sense to me 👍

You don't need the [Skip CI] flag on the commit message Thanks!

minusfive commented 1 year ago

@caksoylar done

caksoylar commented 1 year ago

Thanks again!