-
Notifications
You must be signed in to change notification settings - Fork 304
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
HPCC-29584 Add github action to test helm/k8s #18186
Conversation
https://track.hpccsystems.com/browse/HPCC-29584 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jpmcmu - looks great, a few minor questions, and I'd like @GordonSmith to look it over too.
run: | | ||
k8s_pkg_path=$(ls -t ./${{ inputs.asset-name }}/*.deb 2>/dev/null | head -1) | ||
k8s_pkg_file=$(basename "$k8s_pkg_path") | ||
mv ${k8s_pkg_path} ${{ inputs.platform-folder }}/${k8s_pkg_file} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
curious why package needs to be moved?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, I believe I was doing that to debug an issue and can probably just reference the original path directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jakesmith Turns out this was required. The Dockerfile looks for the pkg file in the platform directory. I added a comment noting why it was required.
sleep 10 | ||
echo "Waiting for ECLWatch startup" && kubectl wait --for=condition=ready pod --timeout=180s -l app=eclwatch | ||
echo "Waiting for Rowservice startup" && kubectl wait --for=condition=ready pod --timeout=180s -l server=rowservice | ||
echo "Waiting for SQL2ECL startup" && kubectl wait --for=condition=ready pod --timeout=180s -l app=sql2ecl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was there a particular reason to wait for this ? and/or not wait for other pods to be ready?
perhaps should be waiting on the deployments rather than individual pods?
Would it be better to wait on the svc ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some tests that I will be adding in a subsequent PR that utilize those services, but it would be better to wait on all deployments to finish. I will make that change.
steps: | ||
- shell: "bash" | ||
run: | | ||
echo "...all tests passed..." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this is just a placeholder in this 'test-regression-suite-k8s' action - and some actual regression suite tests will be run from here in next changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I am planning on adding the HPCC4j unit tests as a regression suite for ESP, but hope this can provide a base for other tests against K8s as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, it will be good to get this foundation merged, which will test k8s startup etc, then we can start to expand to add regression tests.
default: 'docker-ubuntu-22_04-containerized' | ||
|
||
jobs: | ||
build-docker-ubuntu-22_04: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are supporting other "os"s then I would change to something like this:
jobs:
build-docker:
name: build-docker-${{ inputs.os }}:
if: ${{ contains('pull_request,push', github.event_name) }}
uses: ./.github/workflows/build-docker.yml
...
|
||
main: | ||
name: K8s Regression Suite | ||
needs: build-docker-ubuntu-22_04 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above comment
@GordonSmith implemented code review changes, please take a look when you get a chance. |
@jakesmith Implemented code review changes, please review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jpmcmu - looks good.
The "Check that eclhelper interface has not changed / Check eclhelper interface (pull_request)" step is failing, but no idea why, that file hasn't changed, despite what the step details suggest. Could it have accidentally in a push version of this PR?
I'll force a rerun to see if it clears.
- Added a new action to install an HPCC cluster on K8s via Helm - Added a new base regression suite on top of K8s to build-vcpkg Signed-off-by: James McMullan [email protected]
Signed-off-by: James McMullan [email protected]
Type of change:
Checklist:
Smoketest:
Testing: