Allow absolute version file paths

[maennchen]

Description

When using an absolute path like /tmp/.tool-versions, the action would look in the file /$GITHUB_WORKSPACE/tmp/.tool-versions. This change lets it consider absolute paths as well.

• I have performed a self-review of my changes
• I have read and understood the contributing guidelines

[paulo-ferraz-oliveira]

Thanks for this. I'll try to find out why the Actions didn't run, but the change seems sane.

[paulo-ferraz-oliveira]

Would you mind rebasing as a way to force-push something and check what happens with CI? Thanks.

[maennchen]

@paulo-ferraz-oliveira It seems like you don't have on: pr: ... rules in your workflows. It therefore only triggers for pushed onto the original repository.

You need something like this:
https://github.com/erlef/oidcc/blob/366f725b71f9552be5e1d77cc1a108d67237f5d4/.github/workflows/pr.yml#L2

[paulo-ferraz-oliveira]

That makes sense.

[maennchen]

Or just on: [push, workflow_dispatch, pull_request] in the header.

Careful: If you enable push for every branch and pull_request as well, it will run twice if you do a PR from this repo and not a fork.

[maennchen]

I usually do this:

on:
  push:
    branches:
      - main
  pull_request:
    branches:
      - "*"
  workflow_dispatch: {}

[paulo-ferraz-oliveira]

If you enable push for every branch and pull_request as well, it will run twice if you do a PR from this repo and not a fork.

That's what we had before and reverted because we thought only push would do the job.

[paulo-ferraz-oliveira]

I've merged the pull_request stuff to the main branch.

[maennchen]

@paulo-ferraz-oliveira Rebased and it seems to work 👍

[paulo-ferraz-oliveira]

Would you add a simple test? Maybe just take what's there, and extend it to have a file under /tmp and then pass the absolute path.

[maennchen]

@paulo-ferraz-oliveira I've changed it so that i tests both relative and absolute paths.

[paulo-ferraz-oliveira]

There's no need to keep rebase-squashing the changes 😄 (it forces the reviewer to read the commit all over again).

If required (though it's already our policy before merging) we'd kindly ask you to squash everything together.

I'll look at what I have now, though.

[paulo-ferraz-oliveira]

Just two minor comments (not worth tweaking what doesn't need tweaking - since it changes history and makes for harder future spelunking) but Ok otherwise.

[maennchen]

@paulo-ferraz-oliveira I though it makes sense to group the statements together so that it is easier to understand.

I removed the extra lines.

[paulo-ferraz-oliveira]

I though it makes sense to group the statements together so that it is easier to understand.

Yeah, but during a review it only makes it harder:

1. if we want to, there's a unified way to look at all the changes without caring about commits (e.g. https://github.com/erlef/setup-beam/pull/258/files), but
2. if we want to just look at changes since your last commit, it's not easy to follow 😄 (no harm done, I know that every reviewer has their way of looking at things)

[paulo-ferraz-oliveira]

You keep force-pushing even though I've given you reasons and politely asked you not to. 😄

Your changes will be squash-merged before going to the main branch, via GitHub.

[maennchen]

@paulo-ferraz-oliveira Oh, ups, I missed that comment. I'll stop doing that.

[paulo-ferraz-oliveira]

It's Ok. I'll merge now and check if we're Ok to release. Thanks.

[paulo-ferraz-oliveira]

Released in v1.17.4 (v1 and v1.17 reset to the same commit).

[maennchen]

@paulo-ferraz-oliveira Thanks! The tags v1 and v1.17 still refer to a23b1fc though.

[paulo-ferraz-oliveira]

Yeah, I didn't force push 😄 Thanks for noticing.

[paulo-ferraz-oliveira]

Done.