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

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

Closed
TheSpyder opened this issue Jul 24, 2024 · 6 comments · Fixed by #711
Closed

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

TheSpyder opened this issue Jul 24, 2024 · 6 comments · Fixed by #711
Labels
bug Something isn't working

Comments

@TheSpyder
Copy link
Contributor

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

@TheSpyder TheSpyder added the bug Something isn't working label Jul 24, 2024
@miniscruff
Copy link
Owner

miniscruff commented Jul 24, 2024

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
Copy link
Contributor Author

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/[email protected]/command.go:983 +0x840
github.com/spf13/cobra.(*Command).ExecuteC(0x14000146008)
	/home/runner/go/pkg/mod/github.com/spf13/[email protected]/command.go:1115 +0x344
github.com/spf13/cobra.(*Command).Execute(...)
	/home/runner/go/pkg/mod/github.com/spf13/[email protected]/command.go:1039
main.main()
	/home/runner/work/changie/changie/main.go:17 +0x74

Using 1.19.1

@miniscruff
Copy link
Owner

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants