m-lab / ndt

Network Diagnostic Tool
Other
11 stars 7 forks source link

Use canonical build image and local version of NDT #97

Closed stephen-soltesz closed 6 years ago

stephen-soltesz commented 6 years ago

This change addresses build failures discovered in https://github.com/m-lab/ndt/pull/96

The original docker build and run of the ndt_build_and_test.sh script depends on dev branches in both the m-lab/builder and m-lab/ndt-support repositories.

This change removes the dependency on the builder repo (where the dev branch had been deleted) in favor of the pre-built canonical builder docker image, measurementlab/builder:production-1.0.

As well, this change uses the m-lab/ndt-support master repo and replaces the old ndt submodule reference with a local reference to the current TRAVIS_COMMIT.

The result of these changes is that changes to m-lab/ndt will be tested by building the m-lab/ndt-support package using the measurementlab/builder:production-1.0 environment.

The build and test steps in ndt_build_and_test.sh are subsumed by the build step here.


This change is Reviewable

stephen-soltesz commented 6 years ago

:-/ this worked on pushes but does not seem to work as intended on PRs.

stephen-soltesz commented 6 years ago

Now works as intended.

gfr10598 commented 6 years ago

Looks good as far as I can understand, but would like more comments to make it clearer. Not sure if I am misunderstanding what it is doing.


Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.


.travis.yml, line 17 at r2 (raw file):

- git clone https://github.com/m-lab/ndt-support.git
- cd ndt-support
- git config --file=.gitmodules submodule.ndt.url $TRAVIS_BUILD_DIR

I'm not very familiar with submodule stuff in general, and this command in particular. More comments would be helpful. Is this basically pointing at the parent directory?

Perhaps a short illustration of the directory structure and where each bit comes from?


Comments from Reviewable

gfr10598 commented 6 years ago

Reviewed 1 of 1 files at r2. Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

stephen-soltesz commented 6 years ago

No problem. PTAL?


Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.


.travis.yml, line 17 at r2 (raw file):

Previously, gfr10598 (Gregory Russell) wrote…
I'm not very familiar with submodule stuff in general, and this command in particular. More comments would be helpful. Is this basically pointing at the parent directory? Perhaps a short illustration of the directory structure and where each bit comes from?

I've added more comments. Yes, this is basically changing the ndt submodule url from a remote url to the local directory at $TRAVIS_BUILD_DIR.

Is this clear enough without a diagram?


Comments from Reviewable

gfr10598 commented 6 years ago
:lgtm:

Reviewed 1 of 1 files at r3. Review status: all files reviewed at latest revision, 2 unresolved discussions.


.travis.yml, line 17 at r2 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…
I've added more comments. Yes, this is basically changing the ndt submodule url from a remote url to the local directory at $TRAVIS_BUILD_DIR. Is this clear enough without a diagram?

Good enough. Thanks!


.travis.yml, line 18 at r3 (raw file):

- git clone https://github.com/m-lab/ndt-support.git
- cd ndt-support
# Change the ndt-support submodule coniguration for ndt to point to the

coniguration


Comments from Reviewable

stephen-soltesz commented 6 years ago

Thank you!


Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions.


.travis.yml, line 18 at r3 (raw file):

Previously, gfr10598 (Gregory Russell) wrote…
coniguration

Fixed.


Comments from Reviewable

gfr10598 commented 6 years ago
:lgtm:

Review status: 0 of 1 files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable