miniscruff / changie

Automated changelog tool for preparing releases with lots of customization options
https://changie.dev/
MIT License
665 stars 35 forks source link

Invalid kind does not throw an error without a kind format configured #688

Closed TheSpyder closed 1 day ago

TheSpyder commented 1 month ago

Description I have a developer who appears to have hand-crafted a changelog message; I'm still working out what happened but in the meantime I think changie should be failing in this case and it doesn't.

Reproduction Steps I'll attach a full replication zip, but the change file turned out like this:

kind: "Added a whoopsie"
body: 
time: 2024-07-24T12:08:15.373536+10:00

We use changie merge -u '## unreleased during development but changie batch produces the same result.

What happened Normally this invalid kind will cause changie to fail with panic: runtime error: invalid memory address or nil pointer dereference, which is arguably a bug - better validation of change inputs would be nice - but in this case it doesn't fail because we don't use kindFormat, we add the kind to changeFormat to implement our needs described in #608: changeFormat: "- **{{.Kind}}**: {{.Body}}"

What we get is this entry in our changelog:

- **Added a whoopsie**: 

Expected behavior Changie should throw some sort of error, both about the invalid kind and the empty body. Ideally a nice error rather than the panic runtime error.

Additional context kind-replication.zip

miniscruff commented 1 month ago

hmm, changie definitely does validate the kind is valid, if you are using changie new.

▶ changie new -k "Added a whoopsie"
Error: invalid kind: Added a whoopsie
Usage:
  changie new [flags]

Flags:
  -b, --body string        Set the change body without a prompt
  -c, --component string   Set the change component without a prompt
  -m, --custom strings     Set custom values without a prompt
  -d, --dry-run            Print new fragment instead of writing to disk
  -e, --editor             Edit body message using your text editor defined by 'EDITOR' env variable
  -h, --help               help for new
  -k, --kind string        Set the change kind without a prompt
  -j, --projects strings   Set the change projects without a prompt

invalid kind: Added a whoopsie

It is worth mentioning that changie does not do any validation of the fragment at batch time, only at "new" time. Arguably maybe an incorrect design, but that's why you get the output you do.

BodyConfig MinLength is available if you want to enforce body lengths. But again, its only checked at "new" time, not batch.

I was not able to get the panic as you were saying, changie batch auto worked just fine, with and without the kind config.

TheSpyder commented 1 month ago

changie does not do any validation of the fragment at batch time, only at "new" time

Hmm ok I'll keep that in mind. I didn't realise this was the case, that's a bit unfortunate; developers on my team have started using code review to suggest changelog wording fixes directly in the change file, so it's become somewhat normal to hand-edit them.

I do get the error from changie batch auto with that kindFormat uncommented, but turns out changie merge works just fine. It's merge -u that fails.

Full error message:

$ changie merge -u '## unreleased'
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x10 pc=0x102e84d68]

goroutine 1 [running]:
github.com/miniscruff/changie/cmd.(*Batch).WriteChanges(0x1400015ba38, {0x1400022c000?, 0xd?, 0x1b6?})
    /home/runner/work/changie/changie/cmd/batch.go:428 +0x698
github.com/miniscruff/changie/cmd.(*Merge).mergeProject(0x140000a37a0, 0x1400017a788, {0x0, 0x0}, {0x140000b38c0?, 0x0?}, {0x0, 0x0, 0x0?})
    /home/runner/work/changie/changie/cmd/merge.go:136 +0x328
github.com/miniscruff/changie/cmd.(*Merge).Run(0x140000a37a0, 0xe070b86500000000?, {0x1400015bb78?, 0x0?, 0x0?})
    /home/runner/work/changie/changie/cmd/merge.go:75 +0x64
github.com/spf13/cobra.(*Command).execute(0x14000146608, {0x140000931a0, 0x2, 0x2})
    /home/runner/go/pkg/mod/github.com/spf13/cobra@v1.8.0/command.go:983 +0x840
github.com/spf13/cobra.(*Command).ExecuteC(0x14000146008)
    /home/runner/go/pkg/mod/github.com/spf13/cobra@v1.8.0/command.go:1115 +0x344
github.com/spf13/cobra.(*Command).Execute(...)
    /home/runner/go/pkg/mod/github.com/spf13/cobra@v1.8.0/command.go:1039
main.main()
    /home/runner/work/changie/changie/main.go:17 +0x74

Using 1.19.1

miniscruff commented 1 month ago

Hmm ok I'll keep that in mind. I didn't realise this was the case, that's a bit unfortunate; developers on my team have started using code review to suggest changelog wording fixes directly in the change file, so it's become somewhat normal to hand-edit them.

We have sort of done the same, it would probably be ideal to check the fragments at batch time, but it would still pass CI/CD unless you add like a changie batch major --dry-run call to CI. Which idk might be a good idea.

I think the idea of validating fragments at batch time is probably a valid issue. Hard to say if it was a bug or not, but at least its a little less misleading when it comes to what is and isn't validated.

I see in the code where it might panic/nil ref, but for whatever reason its not doing it for me. Weird.

miniscruff commented 1 month ago

hmm, I got the error now, will definitely want to fix this, but it could also be fixed by general fragment validation since its basically just unable to find the right kind to fill in the information for.

TheSpyder commented 1 month ago

Ok, glad you can reproduce now. I had been using changie merge -u as my CI validation, I hadn't considered batch --dry-run. That's a good idea!

miniscruff commented 1 day ago

Example error from #711

Error: kind not found but configuration expects one: 'fixed-typo-checking-ci'
Usage:
  changie batch version|major|minor|patch|auto [flags]
kind not found but configuration expects one: 'fixed-typo-checking-ci'

Flags:
  -d, --dry-run              Print batched changes instead of writing to disk, does not delete fragments
      --footer-path string   Path to version footer file in unreleased directory
  -f, --force                Force a new version file even if one already exists
      --header-path string   Path to version header file in unreleased directory
  -h, --help                 help for batch
  -i, --include strings      Include extra directories to search for change files, relative to change directory
  -k, --keep                 Keep change fragments instead of deleting them
  -m, --metadata strings     Metadata values to append to version
      --move-dir string      Path to move unreleased changes
  -p, --prerelease strings   Prerelease values to append to version
  -j, --project string       Specify which project version we are batching
      --remove-prereleases   Remove existing prerelease versions

exit status 1
Error: Process completed with exit code 1.