Conversation
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]>
cpretzer
left a comment
There was a problem hiding this comment.
Runs great for me locally. Do we need to change anything for *nix based systems?
Signed-off-by: Tarun Pothulapati <[email protected]>
Signed-off-by: Tarun Pothulapati <[email protected]>
Signed-off-by: Tarun Pothulapati <[email protected]>
Signed-off-by: Tarun Pothulapati <[email protected]>
Signed-off-by: Tarun Pothulapati <[email protected]>
Signed-off-by: Tarun Pothulapati <[email protected]>
Signed-off-by: Tarun Pothulapati <[email protected]>
Signed-off-by: Tarun Pothulapati <[email protected]>
## 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]>
Signed-off-by: Tarun Pothulapati <[email protected]>
Signed-off-by: Tarun Pothulapati <[email protected]>
Signed-off-by: Tarun Pothulapati <[email protected]>
.github/workflows/release.yml
Outdated
| - name: Install linkerd CLI | ||
| id: install_cli | ||
| run: | | ||
| $image="gcr.io/linkerd-io/cli-bin:$TAG" |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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! :\
There was a problem hiding this comment.
@Pothulapati note though that you can use bash in windows as well. Worth a shot if the TAG env var is not available 😉
There was a problem hiding this comment.
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-tagResults 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" |
There was a problem hiding this comment.
Have that other docker issue going on, but will want to use $TAG here!
| image="gcr.io/linkerd-io/cli-bin:main" | |
| image="gcr.io/linkerd-io/cli-bin:$TAG" |
There was a problem hiding this comment.
yeah, totally. Just want to check if these commands actually work! wanted to switch to $TAG once I see things work.
d227981 to
1a03acb
Compare
Signed-off-by: Tarun Pothulapati <[email protected]>
|
Updated both |
| - name: Run CLI Integration tests | ||
| run: | | ||
| go test --failfast --mod=readonly ".\test\cli" --linkerd=$HOME\linkerd.exe --cli-tests -v | ||
| kind_integration_tests: |
There was a problem hiding this comment.
Can you please leave the Keep in sync with kind_integration.ymlcomment right abovekind_integration_tests` as well?
.github/workflows/release.yml
Outdated
| 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 |
There was a problem hiding this comment.
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.
Signed-off-by: Tarun Pothulapati <[email protected]>
alpeb
left a comment
There was a problem hiding this comment.
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]>
Signed-off-by: Tarun Pothulapati <[email protected]>
|
@alpeb Yep! That makes total sense. Updated the code to not have a dependency on the docker images. |
alpeb
left a comment
There was a problem hiding this comment.
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`
| - 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: |
There was a problem hiding this comment.
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]>
alpeb
left a comment
There was a problem hiding this comment.
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
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/integrationto separate from/test/clias this test does not fallunder integration tests category
Signed-off-by: Tarun Pothulapati [email protected]