Subtle bug 🔥 in support for ignoring files !?

[favmd]

Dear @rgomezcasas

thanks for testing, refactoring and merging my feature pr #33 (based on issue #9) 👍

But I do think that one of your changes might have added a very subtle bug.

See this code in main:

pr-size-labeler/src/github.sh

Lines 19 to 27 in 88ab4d8

	local changes=0
	for file in $(echo "$body" | jq -r '.[] | @base64'); do
	for file_to_ignore in $files_to_ignore; do
	if [ "$file_to_ignore" != "$(basename $(jq::base64 '.filename'))" ]; then
	total_changes=$(jq::base64 '.changes')
	((changes += total_changes))
	fi
	done
	done

compared to my original version:

    local changes=0


    for file in $(echo "$body" | jq -r '.[] | @base64'); do
      _jq() {
        echo "$file" | base64 -d | jq -r "$1"
      }
      local ignore_file=0 # False
      for file_to_ignore in $files_to_ignore; do
        if [ "$file_to_ignore" = "$(basename $(_jq '.filename'))" ]; then
          ignore_file=1 # True
        fi
      done
      if [ $ignore_file -eq 0 ]; then
        ((changes += $(_jq '.changes')))
      fi
    done

The difference I want to point at is on line 22:

pr-size-labeler/src/github.sh

Line 22 in 88ab4d8

if [ "$file_to_ignore" != "$(basename $(jq::base64 '.filename'))" ]; then

Does this work for files_to_ignore lists with more than one filename?

Let's say that files_to_ignore = 'Pipfile.lock package-lock.json'

and you change your package-lock.json.

1. Your code would check if Pipfile.lock != package-lock.json ?
2. Yes, it is! Count the changes! ~> Err, that's not what we want 🔥

That's why you will have to loop over all filenames all the time.

Note 👉 I haven't tested this, just compared your code to mine and this seemed like a real logical difference.

If you can confirm this, let's do a v1.8.1 asap 🚀

[szamanr]

i can confirm that using v1.8.0 only works with a single filename. i created a PR (in a private repo) with the config changes and some dummy changes in an index.ts file to test the ignore.

• using files_to_ignore: "index.ts" → size label: xs
• using files_to_ignore: "yarn.lock poetry.lock index.ts cz.json de.json en.json es.json fr.json nl.json pt.json sk.json" → size label: xl

@rgomezcasas please release a hotfix.

[favmd]

Thank you @szamanr for verifying this 👍

You're right, it gets even worse with longer lists, counting the changes multiple times.

[danielmklein]

I am not sure if what we are seeing is the same issue or a different issue, but with 1.8.0 our labeling stopped working completely. Rolling back to 1.7.0 fixed things. Here is our job:

    label_size:
        runs-on: ubuntu-latest
        name: Label the PR size
        steps:
            - uses: codelytv/pr-size-labeler@v1.7.0
              with:
                  GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
                  xs_label: 'size/xs'
                  xs_max_size: '10'
                  s_label: 'size/s'
                  s_max_size: '100'
                  m_label: 'size/m'
                  m_max_size: '500'
                  l_label: 'size/l'
                  l_max_size: '1000'
                  xl_label: 'size/xl'
                  fail_if_xl: 'false'
                  message_if_xl: >
                      'Wow, this is a big boi! How might be break something like this into smaller pieces next time?'
                  github_api_url: 'api.github.com'

Could this be due to us not passing files_to_ignore at all?

[favmd]

Dear @danielmklein

That's a very valid question and I can assure you that I had your exact case in mind when testing my feature PR:
People that just link their workflow to the latest available version or v1 and that will thus not pass the new parameter at all. I just tested the version that I currently use for our workflows once more (~> favmd@992a73a), which basically is v1.7.0 + files_to_ignore, and it works: without passing the parameter, with the parameter but empty string, with a one-word list, with a multi-word list (imho that should be all relevant cases).

That being said, I think you might be right in guessing that not passing files_to_ignore causes the failure because of the fact that the diff between the previous version v1.7.0 and the current version v1.8.0 (v1) shows that the new version does handle command line parameters in a fresh new way: ~> v1.7.0...v1.8.0

[davidpilny]

@danielmklein I think you could test by actually passing some files to ignore. For us, it didnt fix the issue.

[rgomezcasas]

@favmd I reverted the code to your original implementation and it works fine 👌

@davidpilny Can you open a new issue with your error attaching your labeler.yml config?

[favmd]

@rgomezcasas Nice 👍 Thanks for letting me know!