orchestration e2e test. #520
Conversation
ser-io
force-pushed
the
ho-integration-test-PR
branch
7 times, most recently
from
a7b60bd
to
416ebb1
Compare
ser-io
requested review from
jemoreira,
ikicha,
0405ysj,
Databean,
rmuthiah,
jmacnak and
adelva1984
as code owners
ser-io
force-pushed
the
ho-integration-test-PR
branch
2 times, most recently
from
356d4a4
to
63d51c1
Compare
ser-io
force-pushed
the
ho-integration-test-PR
branch
6 times, most recently
from
7722377
to
f6ee60c
Compare
ser-io
force-pushed
the
ho-integration-test-PR
branch
4 times, most recently
from
e2bb7f7
to
9460af0
Compare
ser-io
force-pushed
the
ho-integration-test-PR
branch
from
9460af0
to
253a2a9
Compare
| exit 1 | ||
| } | ||
|
|
||
| http_port="61$(($RANDOM%10))$(($RANDOM%10))$(($RANDOM%10))" |
There was a problem hiding this comment.
What if http_port collides?
There was a problem hiding this comment.
in presubmits there should not be collisions as this is the only test requiring port numbers, we should have a more elaborated solution for allocating ports when having more tests down the line. My next step, would be to use Golang to write these tests as we should avoid having more complex logic written in bash.
There was a problem hiding this comment.
Could you leave a TODO comment here, unless you have a plan to do that right after this change? I think it may cause a mysterious error for debuggers if we forget.
There was a problem hiding this comment.
I'm working on the Golang rewrite where we are not going to use randomly picked ports, so this bash code is going away.
| # | ||
|
|
||
| source "shflags" | ||
|
|
||
| DEFINE_boolean verbose "true" "show the stdout/stderr from the sub-tools" "v" | ||
| DEFINE_string out_dir "${PWD}/out" "indicates where to save the built debs" "o" |
There was a problem hiding this comment.
A random idea: Couldn't it be based on the path of build-debs-with-docker.sh, not PWD but $(realpath $(dirname $0))?
Not urgent, but I think this change will make our code better, since we won't need pushd or popd change in the following commit; Make build-debs-with-docker a standalone tool.
There was a problem hiding this comment.
used ${PWD}/out as default for legacy reason
I think the default should be empty and make the flag mandatory, this way the caller always know where the to get the artifacts from, otherwise the caller always need to read into the code to know where the output should be which is not ideal.
There was a problem hiding this comment.
I think the default should be empty and make the flag mandatory
Making the flag as mandatory sounds good. Meaning of my suggestion was, avoiding creating multiple out_dirs depending on the relative path at least.
There was a problem hiding this comment.
this is a valid request which could be implemented later on. for this PR though I wanted to keep the same behavior when none of the flags are passed.
thanks for reviewing! responded to your comments and I'll go ahead and merge the PR. |
- Use `guest_` prefix rather than `_on_guest` suffix.
- Avoid permission errors by mounting a host directory the vsoc-01 user might not have permission to write to, copy the deb files from the guest instead.
- build-debs-with-docker.sh was meant to be run from its parent directory only as part of the `docker/build.sh` tool.
- `bazel build docker/debs-builder-docker:debs_tar` - The new rule generates a tar file with .deb files that could be installed in a docker image.
- `bazel build docker:orchestration_image_tar` - The new rule creates a new docker image with orchestration capabilities.
ser-io
force-pushed
the
ho-integration-test-PR
branch
from
253a2a9
to
2d24cec
Compare
No description provided.