Skip to content

Add a Windows Linkerd cli Test#4653

Merged
Pothulapati merged 21 commits intomainfrom
tarun/cli-win-test
Jul 2, 2020
Merged

Add a Windows Linkerd cli Test#4653
Pothulapati merged 21 commits intomainfrom
tarun/cli-win-test

Conversation

@Pothulapati
Copy link
Contributor

@Pothulapati Pothulapati commented Jun 23, 2020

Fixes #4431

This PR adds a new cli test to see if installation yamls are correctly
generated even on windows, this is important because of all the file
path difference between windows and Linux, and if any code uses a wrong
format might cause the chart generation commands to fail on windows.

This creates a separate workflow for both release, and PR tests, and
downloads the windows binary and runs the test.

Also, all the exisiting integration tests are moved in to
/tests/integration to separate from /test/cli as this test does not fall
under integration tests category

Signed-off-by: Tarun Pothulapati [email protected]

This PR adds a new cli test to see if installation yamls are correctly
generated even on windows, this is important because of all the file
path difference between windows and Linux, and if any code uses a wrong
format might cause the chart generation commands to fail on windows.

This creates a separate workflow for both release, and PR tests, and
downloads the windows binary and runs the test.

Also, all the exisiting integration tests are moved in to
/tests/integration to separate from /test/cli as this test does not fall
under integration tests category

Signed-off-by: Tarun Pothulapati <[email protected]>
Copy link
Contributor

@cpretzer cpretzer left a comment

Choose a reason for hiding this comment

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

Runs great for me locally. Do we need to change anything for *nix based systems?

Signed-off-by: Tarun Pothulapati <[email protected]>
@olix0r olix0r changed the base branch from master to main June 24, 2020 18:36
kleimkuhler added a commit that referenced this pull request Jun 25, 2020
## Description

As discussed [here](#4653 (comment)), the `kind_integration` job of the release workflow was not kept in sync with the changes made in #4593.

Until GitHub actions can reuse yaml for separate workflows, these sections are supposed to be kept in sync.

This would be an issue if we had tried doing a release since #4593 merged, but that has not happened yet.

## Changes

This updates the release workflow `kind_integration` job to use the use new test interface, mainly removing cluster creation and image loading as necessary prerequisites.

Signed-off-by: Kevin Leimkuhler <[email protected]>
- name: Install linkerd CLI
id: install_cli
run: |
$image="gcr.io/linkerd-io/cli-bin:$TAG"
Copy link
Contributor

Choose a reason for hiding this comment

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

Each job needs to set it's own environment variables. $TAG hasn't been set in windows_static_cli_tests yet, so you'll need another copy of the Set environment variables from scripts step in this job!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, @alpeb and I were expecting them to be present throughout the workflow and he is testing it. We previously went with the main tag as its also pushed everytime but then switched over to $TAG.

The problem with running is the script is that its a windows machine and that bash script would not work! :\

Copy link
Member

Choose a reason for hiding this comment

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

@Pothulapati note though that you can use bash in windows as well. Worth a shot if the TAG env var is not available 😉

Copy link
Member

Choose a reason for hiding this comment

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

I tested in a fork using this minimal workflow:

name: Release
on:
  push:
    branches:
    - master
env:
  GH_ANNOTATION: true
  TAG: ""
jobs:
  set_env_var:
    name: Setting TAG
    runs-on: ubuntu-18.04
    steps:
    - name: Checkout code
      # actions/checkout@v2
      uses: actions/checkout@722adc6
    - name: Set environment variables from scripts
      run: |
        . bin/_tag.sh
        echo ::set-env name=TAG::$(CI_FORCE_CLEAN=1 bin/root-tag)
  get_env_var:
    name: Getting TAG (linux)
    needs: [set_env_var]
    runs-on: ubuntu-18.04
    steps:
    - name: Check TAG using bash
      shell: bash
      run: |
        echo "TAG:"
        echo $TAG
  windows_static_cli_tests:
    name: Getting TAG (windows)
    needs: [set_env_var]
    runs-on: windows-latest
    steps:
    - name: Check TAG using bash
      shell: bash
      run: |
        echo "TAG:"
        echo $TAG
    - name: Check TAG using PS
      run: |
        echo "TAG:"
        echo $env:TAG
    - name: Checkout code
      # actions/checkout@v2
      uses: actions/checkout@722adc6
    - name: Run bin/root-tag
      shell: bash
      run: bin/root-tag

Results here.

As @Pothulapati noticed before, global env vars are only for static values; attempting to change them via echo ::set-env name=TAG::... doesn't work. That only changes the value for the current job and the value isn't carried over the following jobs, regardless of them using linux or windows (PS or bash).
However, if we set the shell to be bash, we can run bin/root-tag to retrieve the tag!

run: |
echo ::set-env name=TAG::$(CI_FORCE_CLEAN=1 bin/root-tag)
echo $TAG
image="gcr.io/linkerd-io/cli-bin:main"
Copy link
Contributor

Choose a reason for hiding this comment

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

Have that other docker issue going on, but will want to use $TAG here!

Suggested change
image="gcr.io/linkerd-io/cli-bin:main"
image="gcr.io/linkerd-io/cli-bin:$TAG"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, totally. Just want to check if these commands actually work! wanted to switch to $TAG once I see things work.

@Pothulapati
Copy link
Contributor Author

Updated both release.yaml & kind_integration.yaml to be the same by adding create and upload artifacts to release.yaml.

- name: Run CLI Integration tests
run: |
go test --failfast --mod=readonly ".\test\cli" --linkerd=$HOME\linkerd.exe --cli-tests -v
kind_integration_tests:
Copy link
Member

Choose a reason for hiding this comment

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

Can you please leave the Keep in sync with kind_integration.ymlcomment right abovekind_integration_tests` as well?

Comment on lines 50 to 60
for image in proxy controller web cni-plugin debug cli-bin grafana; do
docker save "$DOCKER_REGISTRY/$image:$TAG" > $ARCHIVES/$image.tar || tee save_fail &
done

# save cli's as artifacts
cp -r ./target/cli $ARCHIVES/cli

# Wait for `docker save` background processes to complete. Exit early
# if any job failed.
wait < <(jobs -p)
test -f save_fail && exit 1 || true
Copy link
Member

@alpeb alpeb Jul 1, 2020

Choose a reason for hiding this comment

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

IIUC we only need the CLI image, so you can simplify all this with a single docker save and there's no need to do the process waiting business.

Copy link
Member

@alpeb alpeb left a comment

Choose a reason for hiding this comment

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

Actually, in release.yml we don't need the new "Set environment variables from scripts" step, and the "Create artifact with CLI image archive" should only copy the windows binary and we don't need to upload any image as an artifact. You could also append the .exe extension while doing the copying, thus saving us from that "Install CLI" step down below (this also applies to kind_integration.yml).

Signed-off-by: Tarun Pothulapati <[email protected]>
Signed-off-by: Tarun Pothulapati <[email protected]>
@Pothulapati
Copy link
Contributor Author

@alpeb Yep! That makes total sense. Updated the code to not have a dependency on the docker images.

Copy link
Member

@alpeb alpeb left a comment

Choose a reason for hiding this comment

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

While testing the release workflow in my fork the windows test is failing with:

can't load package: package ./test/cli: cannot find package "." in:
	D:\a\linkerd2\linkerd2\test\cli`

https://github.com/alpeb/linkerd2/runs/831033791?check_suite_focus=true

- name: Run CLI Integration tests
run: |
go test --failfast --mod=readonly ".\test\cli" --linkerd=$PWD\image-archives\linkerd-windows.exe --cli-tests -v
kind_integration_tests:
Copy link
Member

Choose a reason for hiding this comment

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

Can you add "# todo: Keep in sync with release.yml" back here (while leaving it for windows_static_cli_tests).

Signed-off-by: Tarun Pothulapati <[email protected]>
Signed-off-by: Tarun Pothulapati <[email protected]>
Copy link
Member

@alpeb alpeb left a comment

Choose a reason for hiding this comment

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

My bad, I had failed to move the tests into the new dir when I tested in the fork. Working great now!
https://github.com/alpeb/linkerd2/runs/831610367?check_suite_focus=true

@Pothulapati Pothulapati merged commit cf34a14 into main Jul 2, 2020
@Pothulapati Pothulapati deleted the tarun/cli-win-test branch July 2, 2020 17:43
@Pothulapati Pothulapati mentioned this pull request Jul 2, 2020
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.

ci: Test windows cli

4 participants