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

RC for V1.2 #3

Merged
merged 21 commits into from
Dec 11, 2023
Merged

RC for V1.2 #3

merged 21 commits into from
Dec 11, 2023

Conversation

GuillaumeFalourd
Copy link
Owner

@GuillaumeFalourd GuillaumeFalourd commented Sep 19, 2023

Description

Closes #2

  • Updating the action.yaml file to support Windows OS runners back

Evidences

https://github.com/GuillaumeFalourd/setup-rsync/actions/runs/6253144375

Signed-off-by: Guillaume Falourd <guillaume.falourd@zup.com.br>
Signed-off-by: Guillaume Falourd <guillaume.falourd@zup.com.br>
Signed-off-by: Guillaume Falourd <guillaume.falourd@zup.com.br>
@GuillaumeFalourd GuillaumeFalourd self-assigned this Sep 19, 2023
@GuillaumeFalourd GuillaumeFalourd mentioned this pull request Sep 19, 2023
@koppor
Copy link
Contributor

koppor commented Sep 20, 2023

I tested it. Could not get it to work.

Do you have a hint how to get it running with an ssh-key?


Some notes:

sshkey: If the key is written to ./sshkey and chmod 600 ./sshkey it does not work:

Permissions for 'sshkey' are too open.

I tried to use bash for both generation of the key file (/tmp/sshkey) and for executing rsync. If rsync was executed in cmd.exe, it could not access /tmp/sshkey. However, the bash one also could not access that file. Maybe /tmp is cleaned up between sessions?

I tried ~/sshkey as SSH key file: /home/runneradmin/sshkey - No such file or directory

(Other hints on ssh keys are given at https://maxschmitt.me/posts/github-actions-ssh-key - however, I don't want to use an agent)


Just as reference - with chocolatey, everything works:

rsync installation takes about 30 seconds ( choco install rsync):

image

There, the permission issue with the ssh key does not occur.

Part of the command:

-e 'C:\ProgramData\chocolatey\lib\rsync\tools\bin\ssh.exe -i sshkey -o StrictHostKeyChecking=no'

Note the special location of ssh.exe. Source: https://serverfault.com/a/1053949/107832

Maybe, you could switch from the "invasive" cygwin installation to a simple choco installation? Maybe, the action can be paramterized: windowsdistribution: cygwin|chocolatey, which cygwin as default (to be backward compatible).

@koppor
Copy link
Contributor

koppor commented Sep 20, 2023

I gave up with cygwin and ssh. Tried around with '${{runner.temp}}/sshkey', but still

Warning: Identity file D:\a\_temp/sshkey not accessible: No such file or directory.

With some more work:

      - name: Setup ssh key
        shell: bash
        run: |
          temp_path="${{ runner.temp }}"
          temp_path="${temp_path//\\//}"
          echo "${{ secrets.SSH_KEY }}" > "${temp_path}/sshkey"
          chmod 600 "${{runner.temp}}/sshkey"
      - name: rsync
        shell: bash
        run: |
          temp_path="${{ runner.temp }}"
          sshkeypath="${temp_path//\\//}/sshkey"
          rsync -Pavz --itemize-changes --stats --partial-dir=/tmp/partial --rsync-path="mkdir -p /tmp/test && rsync" -e "ssh -i ${sshkeypath} -o StrictHostKeyChecking=no" build/distribution/ user@example.org:/tmp/test/

But then

D:\a\_temp\0628137d-dcd0-4df7-88cf-7c626c38454f.sh: line 30: D:/a/_temp

/sshkey

Will use choco install rsync

@koppor
Copy link
Contributor

koppor commented Sep 20, 2023

My job:

jobs:
  rsynctest:
    strategy:
      fail-fast: false
      matrix:
        os: [ubuntu-latest, windows-latest, macos-latest]
    runs-on: ${{ matrix.os }}
    steps:
      - name: Setup rsync
        uses: GuillaumeFalourd/setup-rsync@test
      - name: Setup ssh key
        shell: bash
        run: |
          temp_path="${{ runner.temp }}"
          temp_path="${temp_path//\\//}"
          echo "${{ secrets.SSH_KEY }}" > "${temp_path}/sshkey"
          chmod 600 "${{runner.temp}}/sshkey"
          mkdir -p build/distribution/
          touch build/distribution/test.txt
      - name: rsync
        shell: bash
        run: |
          temp_path="${{ runner.temp }}"
          sshkeypath="${temp_path//\\//}/sshkey"
          rsync -Pavz --itemize-changes --stats --partial-dir=/tmp/partial --rsync-path="mkdir -p /tmp/test && rsync" -e "ssh -i ${sshkeypath} -o StrictHostKeyChecking=no" build/distribution/ user@example.org:/tmp/test/

@GuillaumeFalourd
Copy link
Owner Author

GuillaumeFalourd commented Sep 20, 2023

Hi @koppor , thank you for the insights!

I'll move the action to use choco instead for windows in that case.

EDIT: Seems to work, as can be seen here

Would you find it helpful to have a not mandatory ssh key input to execute the following command during the action execution?

     run: |
          temp_path="${{ runner.temp }}"
          temp_path="${temp_path//\\//}"
          echo "${{ secrets.SSH_KEY }}" > "${temp_path}/sshkey"
          chmod 600 "${{runner.temp}}/sshkey"

Signed-off-by: Guillaume Falourd <guillaume.falourd@zup.com.br>
Signed-off-by: Guillaume Falourd <guillaume.falourd@zup.com.br>
Copy link
Contributor

@koppor koppor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some small comments.

I would also modify the test workflow - one can use a local action.yml after checkout as action. See https://github.com/orgs/community/discussions/26245 for details.

action.yml Outdated Show resolved Hide resolved
action.yml Outdated
Comment on lines 44 to 46
echo "ubuntu runner detected"
sudo apt-get install rsync
echo "rsync installed on ubuntu runner"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This always outputs that rsync is installed. Maybe, skip installation - and if GitHub updates the image - and removes rsync, this action can be updated.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would still let this part here as this action could be used on self-hosted runner that may not have rsync installed by default.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does one need apt-get update before? - On "normal" docker images, this is required, because the indexes are normally wiped away to save space.

Some bookmarks to share:

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've update this section in the action.yaml file if you want to have a look 👍🏼

else
temp_path="${{ runner.temp }}"
temp_path="${temp_path//\\//}"
echo "${{ inputs.ssh_key }}" > "${temp_path}/sshkey"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, this did not work on Cygwin bash. Hopefully, it does on git bash.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to identify if it's using cygwin or git before executing those commands?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The environment variable CYGWIN is set in the case of the former.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've update this section in the action.yaml file if you want to have a look 👍🏼

@koppor koppor mentioned this pull request Sep 20, 2023
@koppor
Copy link
Contributor

koppor commented Sep 20, 2023

Would you find it helpful to have a not mandatory ssh key input to execute the following command during the action execution?

Sure!

It would also be great if the path to the sshkey would be available as output of the action. In that way, one can "easily" use it as input for -i for rsync.

action.yml Outdated Show resolved Hide resolved
Co-authored-by: Oliver Kopp <kopp.dev@gmail.com>
@GuillaumeFalourd
Copy link
Owner Author

GuillaumeFalourd commented Sep 20, 2023

Would you find it helpful to have a not mandatory ssh key input to execute the following command during the action execution?

Sure!

It would also be great if the path to the sshkey would be available as output of the action. In that way, one can "easily" use it as input for -i for rsync.

I'll add it 👍🏼

EDIT: Done

GuillaumeFalourd and others added 12 commits September 20, 2023 10:38
Signed-off-by: Guillaume Falourd <guillaume.falourd@zup.com.br>
Signed-off-by: Guillaume Falourd <guillaume.falourd@zup.com.br>
Signed-off-by: Guillaume Falourd <guillaume.falourd@zup.com.br>
Signed-off-by: Guillaume Falourd <guillaume.falourd@zup.com.br>
Signed-off-by: Guillaume Falourd <guillaume.falourd@zup.com.br>
Signed-off-by: Guillaume Falourd <guillaume.falourd@zup.com.br>
Signed-off-by: Guillaume Falourd <guillaume.falourd@zup.com.br>
Signed-off-by: Guillaume Falourd <guillaume.falourd@zup.com.br>
Signed-off-by: Guillaume Falourd <guillaume.falourd@zup.com.br>
Signed-off-by: Guillaume Falourd <guillaume.falourd@zup.com.br>
action.yml Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
GuillaumeFalourd and others added 3 commits December 11, 2023 11:44
Co-authored-by: Oliver Kopp <kopp.dev@gmail.com>
Co-authored-by: Oliver Kopp <kopp.dev@gmail.com>
Co-authored-by: Oliver Kopp <kopp.dev@gmail.com>
@GuillaumeFalourd GuillaumeFalourd merged commit c16d3c9 into main Dec 11, 2023
4 checks passed
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 this pull request may close these issues.

Fails on Windows
2 participants