-
Notifications
You must be signed in to change notification settings - Fork 19
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
Run Skupper integration tests with the skupper-router image #169
base: main
Are you sure you want to change the base?
Run Skupper integration tests with the skupper-router image #169
Conversation
a46e86d
to
b29e6d5
Compare
That happened because the image is only present among |
That is because the systemd gateway test expects to find I'm inclined to leave this test skipped, because installing dispatch is inconvenient... |
skupperGitRef: | ||
required: false | ||
description: Reference to skupper version | ||
default: "0.8.7" |
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.
why not leaving master
as the default?
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.
My thinking is that we want to catch issues in skupper-router here. If both skupper and skupper-router are changing from one run of the job to the next, we will never be able to guess which is the culprit of a failure.
So, I want to have the latest known working version of skupper here. Using latest release is the only way I can think of to get a working version. I could somehow figure out how to find the latest commit on master
that passed all skupper tests, for example... Or possibly even something more complicated.
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.
Ok, it might be better using 0.8
instead, in this case.
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.
Certainly not. There has to be a skupper version that works with the skupper-router after the @kgiusti 's renaming. So, at first, I expect there will have to be a commit hash of whatever version where you fix skupper for it. Then, going forward, we can pick some suitable release, yes.
export PUBLIC_1_INGRESS_HOST=10.0.1.1 | ||
export QDROUTERD_IMAGE=localhost:32000/skupper-router:registry | ||
|
||
go test -count=1 -p=1 -timeout=60m -tags=integration -v ./test/integration/... |
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.
you can remove -p=1
here, which would speed things up
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.
That is a good call, I probably should do that. I worried the output would become less clear, but the way it is now is simply too slow to be useful. Removing -p1
it is.
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.
-p=3 now, and it passed a few times in a row
7b4aca2
to
9bd3d34
Compare
a1b9cbc
to
dec746f
Compare
f870566
to
4c764e4
Compare
5c4b31b
to
03756de
Compare
At this point, there is one fundamental question that needs to be asked. @kgiusti @ganeshmurthy (@cliffjansen) can you live with a CI test that runs 40 minutes and if it fails, it is quite unclear why it failed? (even more unclear than the system-tests)? To summarize, this brings in integration-tests from skupper, from some known-working version of skupper, but puts in the latest skupper-router image. Then the tests run. These are the same tests that Renato is (was?, still is?) so unhappy about. |
e83c251
to
9063ba3
Compare
fcfbdd1
to
7d51cc0
Compare
e178956
to
051bbdd
Compare
92197e6
to
a4f8062
Compare
a4f8062
to
c326dfe
Compare
89530c7
to
8fff663
Compare
8aadf81
to
f12570c
Compare
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## main #169 +/- ##
=======================================
Coverage 30.85% 30.85%
=======================================
Files 174 174
Lines 38052 38052
Branches 5331 5331
=======================================
Hits 11741 11741
Misses 25155 25155
Partials 1156 1156
Flags with carried forward coverage won't be shown. Click here to find out more. Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
@fgiorgetti does this by any chance mean anything to you? I'm thinking maybe I'm missing something obvious. If not, I will consider investigating further (or maybe burying this PR and never coming back ;) |
Build the image for pull requests. Only push the image if we are on
main
branch. Run skupper integration tests with the image before push.