Move main script to main.sh

[connortann]

Moving main action to a dedicated shell script, to enable the ShellCheck linter to function on CI.

I have found a way to call a shell script from within the action; the slight complication is that input parameters need to be mapped as environment variables.

The script is unchanged other than replacing ${{ inputs.foo-bar }} with ${FOO_BAR} in relevant places.

I would appreciate a code review of how bash variables are used (i.e. to ensure correct use of quotes)

[connortann]

Any assistance fixing the failing test would be greatly appreciated! I will have some more time to look at this next week hopefully.

It appears to be due to path expansion. Might it be a "False Positive" failure? EDIT: fixed now

# test-non-default-settings
assertion failed: /home/runner/.cache/virtualenvs not found in ~/.cache/virtualenvs

[sondrelg]

Yes looks like adjusting the test makes most sense. I'll try my best to find time to review this tomorrow or Saturday. Looks promising 👍 Thanks for the effort @connortann!

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[connortann]

This should be ready for your review now. Apologies for all the force-pushes, I've been moving commits between different branches to test things on CI.

The test suite now passes without modification: I've ensured tildes ~ in virtualenvs.path are still expanded, as I noticed that was previously an issue #54 .

[sondrelg]

Changes look good, and happy to use env vars since we're otherwise fully backwards compatible wrt. inputs.

I just have two questions. The first is regarding the naming of the env vars - see the comment below. The second has to do with GITHUB_ACTION_PATH:

Have you seen docs explicitly stating that the GITHUB_ACTION_PATH will link back to the commit related to each release? In other words, if we update main.sh in the future, will users running v1.3.1 still retrieve the v1.3.1-version of main.sh? I don't think anything else makes sense, but I couldn't find anything to confirm it 🙂

[sondrelg]

Poetry supports environment variables for settings (docs). I wonder if we might be able to kill two birds with one stone here. Perhaps we can rename the env vars to POETRY_* and drop code like this from the shell script completely

$poetry_ config virtualenvs.create ${VIRTUALENVS_CREATE}
$poetry_ config virtualenvs.in-project ${VIRTUALENVS_IN_PROJECT}
$poetry_ config virtualenvs.path ${VIRTUALENVS_PATH}

Needs testing, but unless I'm mistaken this should be inferred by Poetry if we do this, so the extra specification will become redundant.

[sondrelg]

If this works out, I think it might also be worth checking if a user can overwrite our env vars using the env tag when specifying the action, like this:

- uses: snok/install-poetry@v1
  with:
    version: 1.1.10
  env:
    POETRY_VERSION: 1.1.13

It would be great if the POETRY_VERSION here would take precedence, but I'm guessing it wouldn't.

[sondrelg]

Assuming the condition becomes redundant if we use POETRY_ env vars

Suggested change

	if [ "${VERSION}" == "latest" ]; then
	POETRY_HOME=$path python3 $installation_script --yes ${INSTALLATION_ARGUMENTS}
	else
	POETRY_HOME=$path python3 $installation_script --yes --version=${VERSION} ${INSTALLATION_ARGUMENTS}
	fi
	POETRY_HOME=$path python3 $installation_script --yes ${INSTALLATION_ARGUMENTS}

[sondrelg]

On second thought, I'm not so sure the version will be inferred by the installer. I can help test tomorrow.

[sondrelg]

POETRY_VERSION=1.1.10 python3 $installation_script --yes prompts for a 1.1.10 installation, so that seems like it's working 👍

[sondrelg]

Assuming this all becomes redundant if we use POETRY_ env vars

Suggested change

	$poetry_ config virtualenvs.create ${VIRTUALENVS_CREATE}
	$poetry_ config virtualenvs.in-project ${VIRTUALENVS_IN_PROJECT}
	$poetry_ config virtualenvs.path ${VIRTUALENVS_PATH}
	config="$($poetry_ config --list)"
	if echo "$config" | grep -q -c "installer.parallel"; then
	$poetry_ config installer.parallel "${INSTALLER_PARALLEL}"
	fi

[sondrelg]

While using POETRY_VERSION with the installation script works, doing POETRY_VIRTUALENVS_CREATE=false python3 $installation_script --yes does not results in the correct configuration:

❯ /Users/sondrelg/.local/bin/poetry config --list
cache-dir = "/Users/sondrelg/Library/Caches/pypoetry"
experimental.new-installer = true
installer.parallel = true
virtualenvs.create = true  # <-- should be false 
virtualenvs.in-project = null
virtualenvs.path = "{cache-dir}/virtualenvs"  # /Users/sondrelg/Library/Caches/pypoetry/virtualenvs

I suppose these values are checked when running poetry install, in which case I guess we would still need to explicitly set the configuration here, or export the environment variables so they're available in subsequent steps.

[sondrelg]

Looking at it again, I think the env vars might be a good idea, but probably not for this PR. I'll merge this as is. Thanks @connortann 👏

[connortann]

Thanks for your review @sondrelg !

I love your idea of using poetry environment variables, that would be a great simplification if it works. Could be a good future refactor.

Regarding GITHUB_ACTION_PATH, no I didn't see a confirmation in the docs that it points to the correct commit; however, I found it mentioned here as a recommended way to access files in the action repo, and it's also used in the tutorial. So, this seems like a reasonably safe thing to do: as you say I don't think anything else would make sense.