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

Error applying minor version label #1011

Closed
KevinBatdorf opened this issue Jan 13, 2022 · 17 comments · Fixed by #1012
Closed

Error applying minor version label #1011

KevinBatdorf opened this issue Jan 13, 2022 · 17 comments · Fixed by #1012

Comments

@KevinBatdorf
Copy link

Got an error when the workflow had a minor label applied

Screen Shot 2022-01-13 at 12 13 55 PM

name: Draft Release
on:
    push:
        branches:
            - main
        pull_request:
            types: [opened, reopened, synchronize]

jobs:
    update_release_draft:
        runs-on: ubuntu-latest
        steps:
            - uses: release-drafter/release-drafter@v5
              with:
                  config-name: release-template.yml
              #   disable-autolabeler: true
              env:
                  GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}

name-template: '$RESOLVED_VERSION'
tag-template: '$RESOLVED_VERSION'
categories:
    - title: "🚀 What's new"
      labels:
          - 'PR: New'
    - title: '🏋️ Improvements'
      labels:
          - 'PR: Improvement'
    - title: '🐛 Bug Fixes'
      labels:
          - 'PR: Fix'
exclude-labels:
    - 'PR: No Changelog'
change-template: '- $TITLE @$AUTHOR (#$NUMBER)'
change-title-escapes: '\<*_&' # You can add # and @ to disable mentions, and add ` to disable code blocks.
version-resolver:
    minor:
        labels:
            - 'minor'
    patch:
        labels:
            - 'patch'
    default: patch
template: |
    ## Release Changes

    $CHANGES

I have a general question to. If we add a minor label to multiple PRs will it continuously bump the version number? Or is it only one bump per release? Thanks

@jetersen
Copy link
Member

Instead of a screenshot, can you please go to the github action and take the text form?

@jetersen
Copy link
Member

jetersen commented Jan 13, 2022

I have a general question to. If we add a minor label to multiple PRs will it continuously bump the version number? Or is it only one bump per release? Thanks

We only do a single increment.

@KevinBatdorf
Copy link
Author

Here's the stack trace:

stack: "TypeError: Cannot read properties of undefined (reading 'labels')\n" +
    '    at /home/runner/work/_actions/release-drafter/release-drafter/v5/dist/index.js:133627:41\n' +
    '    at Array.flatMap (<anonymous>)\n' +
    '    at resolveVersionKeyIncrement (/home/runner/work/_actions/release-drafter/release-drafter/v5/dist/index.js:133626:8)\n' +
    '    at generateReleaseInfo (/home/runner/work/_actions/release-drafter/release-drafter/v5/dist/index.js:133678:5)\n' +
    '    at drafter (/home/runner/work/_actions/release-drafter/release-drafter/v5/dist/index.js:132984:25)\n' +
    '    at async Promise.all (index 0)',
  type: 'Error'

I shared the screenshot so you knew the context of where the error was being reported. Thought it would be easy enough to find where labels is being accessed on a possible undefined value. My guess is here:

.flatMap((pr) => pr.labels.nodes.map((node) => labelToKeyMap[node.name]))

@darrentorpey
Copy link

Just want to drop a quick note here to say that our repos that use release-drafter/release-drafter@v5 have started to get what seems to be the same error:

  stack: 'AggregateError: \n' +
    "    TypeError: Cannot read properties of undefined (reading 'labels')\n" +
    '        at /home/runner/work/_actions/release-drafter/release-drafter/v5/dist/index.js:133627:41\n' +
    '        at Array.flatMap (<anonymous>)\n' +
    '        at resolveVersionKeyIncrement (/home/runner/work/_actions/release-drafter/release-drafter/v5/dist/index.js:133626:8)\n' +
    '        at generateReleaseInfo (/home/runner/work/_actions/release-drafter/release-drafter/v5/dist/index.js:133678:5)\n' +
    '        at drafter (/home/runner/work/_actions/release-drafter/release-drafter/v5/dist/index.js:132984:25)\n' +
    '        at async Promise.all (index 0)\n' +
    '    at /home/runner/work/_actions/release-drafter/release-drafter/v5/dist/index.js:16687:19',

and we are not using a "minor" label, so that detail is probably not central to the breakage.

I have restored things to working order in a repo by locking the version to 5.15.0 so one of the two recent releases must have broken this.

For those who want to use this workaround, here's what I did:

I changed

        uses: release-drafter/release-drafter@v5

to

        uses: release-drafter/release-drafter@v5.15.0

and on my next merge to my main branch all worked as expected.

(heads up @KevinBatdorf in case this helps you)

@jetersen
Copy link
Member

jetersen commented Jan 14, 2022

@darrentorpey I reverted v5 tag to point to v5.15.0 so it should be fine.

So our tests are passing but I guess we do not have a tests with pull requests that has no labels 😅
Will look into adding a test and making a fix later this week.

I shared the screenshot so you knew the context of where the error was being reported. Thought it would be easy enough to find where labels is being accessed on a possible undefined value.

Nope screenshot is the worst form of info sharing :D I cannot see the surrounding context.
The stack trace means I can use a source map to see exactly where it goes wrong :)

@KevinBatdorf
Copy link
Author

we do not have a tests with pull requests that has no labels

The PR where it failed for me had a label on it. Here's another screenshot for ya:

Screen Shot 2022-01-13 at 8 16 06 PM

Possibly it's the exclude-labels label causing the regression?

@jetersen
Copy link
Member

jetersen commented Jan 14, 2022

The line number :133627 inside dist/index.js is here:

config['version-resolver'][key].labels.map((label) => [label, key]),

Which is unchanged for the past 2 years 🗡️

I see we have zero test coverage of this function apart from the index tests that touches it.

@KevinBatdorf
Copy link
Author

Odd. I'd offer to help test a fix you might be thinking of but not sure how I could. The two files I shared at the start are unedited so maybe it's reproducible? That plus the labels on the PR seem like everything involved. We'll likely do another release next week or the following and I can report back then too.

Must be a change after 5.15.0 though. Maybe a side effect from an object mutation somewhere else?

@jetersen
Copy link
Member

Could be related to #973 but even than the default config has empty labels for each key 😖

@KevinBatdorf
Copy link
Author

I'm just throwing out ideas but maybe this change to a Set caused a regression elsewhere?

1f825d3#diff-191fd5fc5599fadfab5a79d6590a13d1c252404a84be653223c41c381c55498bR124-R126

@jetersen
Copy link
Member

We are using version-resolver and dogfooding our action on master branch and the action passes:
https://github.com/release-drafter/release-drafter/runs/4811849003?check_suite_focus=true

😕

version-resolver:
major:
labels:
- 'type: breaking'
minor:
labels:
- 'type: feature'
patch:
labels:
- 'type: bug'
- 'type: maintenance'
- 'type: docs'
- 'type: dependencies'
- 'type: security'

@KevinBatdorf
Copy link
Author

Have you tried a PR with only the labels type: feature and skip-changelog? Because that would possibly match the scenario that failed for me.

If still ok, then it would have to be the other config items that differ, right? I mostly used boilerplate from the example I think.

@jetersen
Copy link
Member

@KevinBatdorf if you add a major section to your version-resolver I suspect it will work.

I believe it is #973 it is not performing a deep merge of the defaults. So it should have defaulted to version-resolver: { major: labels: [], }

@jetersen
Copy link
Member

Yup a unit test is able to reproduce your error message with a partial config.

@KevinBatdorf
Copy link
Author

Would it be better to make that an optional property you think?

@jetersen
Copy link
Member

jetersen commented Jan 15, 2022

Nah, the default config ensure our code base works as expected on objects without too much conditional code logic :)

See #1012

@jetersen
Copy link
Member

v5 is updated with the fix :)

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 a pull request may close this issue.

3 participants