search

thaliproject / CI

CI project for testing mobile devices
MIT License
2 stars 3 forks source link

update CI #83

Closed larryonoff closed 7 years ago

larryonoff commented 7 years ago

This change is Reviewable

larryonoff commented 7 years ago

@mlesnic @czyzm @jareksl @yaronyg I just collected the changes (not all) on CI that weren't committed and created PR for this. Please review this PR.

czyzm commented 7 years ago

Reviewed 2 of 5 files at r1. Review status: 2 of 5 files reviewed at latest revision, 1 unresolved discussion.

_tasker/check_node.sh, line 1 at r1 (raw file):_

#!/usr/bin/env bash

This and the next one are the files added long long time ago by Ville to help with diagnostic of nodes and connected devices. I think it is good to have them in repo.

Comments from Reviewable

czyzm commented 7 years ago

Review status: 2 of 5 files reviewed at latest revision, 3 unresolved discussions.

builder/build__.sh, line 30 at r1 (raw file):

cd Github;ERROR_ABORT
rm -rf testBuild;ERROR_ABORT
cp -r testBuildOrg/ testBuild;

Wondering why ERROR_ABORT was removed from this item. I would try keeping it and run some build.

_builder/virtual.js, line 93 at r1 (raw file):_

    data = data.replace("{{BUILD_SCRIPT_PATH}}", scr);

    if (job.test) {

This is some leftover from CI smoke tests. I don't see it is used anywhere so remove it.

Comments from Reviewable

czyzm commented 7 years ago

Reviewed 3 of 5 files at r1. Review status: all files reviewed at latest revision, 3 unresolved discussions.

Comments from Reviewable

larryonoff commented 7 years ago

Review status: all files reviewed at latest revision, 3 unresolved discussions.

builder/build__.sh, line 30 at r1 (raw file):

Previously, czyzm wrote… > Wondering why ERROR_ABORT was removed from this item. I would try keeping it and run some build. >

let's wait what @mlesnic tells.

_builder/virtual.js, line 93 at r1 (raw file):_

Previously, czyzm wrote… > This is some leftover from CI smoke tests. I don't see it is used anywhere so remove it. >

let's wait what @mlesnic tells.

_tasker/check_node.sh, line 1 at r1 (raw file):_

Previously, czyzm wrote… > This and the next one are the files added long long time ago by Ville to help with diagnostic of nodes and connected devices. I think it is good to have them in repo. >

Done.

Comments from Reviewable

larryonoff commented 7 years ago

Review status: all files reviewed at latest revision, 3 unresolved discussions.

builder/build__.sh, line 30 at r1 (raw file):

Previously, larryonoff (larryonoff) wrote… > let's wait what @mlesnic tells. >

I agree that it's strange removing this check.

Comments from Reviewable

yaronyg commented 7 years ago

:lgtm: Obviously my lgtm isn't worth as much as the Rockwell folks.

Reviewed 5 of 5 files at r1. Review status: all files reviewed at latest revision, 3 unresolved discussions.

builder/build__.sh, line 30 at r1 (raw file):

Previously, larryonoff (larryonoff) wrote… > I agree that it's strange removing this check. >

Rather than using ERROR_ABORT everywhere I suggest instead using 'set -euo pipefail'. That will catch any error in any situation and make things fail. And it doesn't require peppering everything with ';ERROR_ABORT'. I would suggest however also adding https://github.com/thaliproject/Thali_CordovaPlugin/blob/iOS/build.sh#L16-L26 which will give you a nice output with which command failed.

Comments from Reviewable

lesn1kk commented 7 years ago

Review status: all files reviewed at latest revision, 2 unresolved discussions.

builder/build__.sh, line 30 at r1 (raw file):

Previously, yaronyg (Yaron Y Goland) wrote… > Rather than using ERROR_ABORT everywhere I suggest instead using 'set -euo pipefail'. That will catch any error in any situation and make things fail. And it doesn't require peppering everything with ';ERROR_ABORT'. I would suggest however also adding https://github.com/thaliproject/Thali_CordovaPlugin/blob/iOS/build.sh#L16-L26 which will give you a nice output with which command failed. >

This was removed due to some problem with builds in the past. I believe it was something with permissions, like what I did lately. Current snapshot should be ok so we can bring ERROR_ABORT back.

_builder/virtual.js, line 93 at r1 (raw file):_

Previously, larryonoff (larryonoff) wrote… > let's wait what @mlesnic tells. >

Yes, please remove it.

Comments from Reviewable

larryonoff commented 7 years ago

Review status: 3 of 11 files reviewed at latest revision, 2 unresolved discussions.

builder/build__.sh, line 30 at r1 (raw file):

Previously, mlesnic (Marcin Lesniczek) wrote… > This was removed due to some problem with builds in the past. I believe it was something with permissions, like what I did lately. Current snapshot should be ok so we can bring ERROR_ABORT back. >

I've migrated all shell scripts out of ERROR_ABORT

Comments from Reviewable

larryonoff commented 7 years ago

Review status: 3 of 11 files reviewed at latest revision, 2 unresolved discussions.

_builder/virtual.js, line 93 at r1 (raw file):_

Previously, mlesnic (Marcin Lesniczek) wrote… > Yes, please remove it. >

Done.

Comments from Reviewable

larryonoff commented 7 years ago

@czyzm @mlesnic please review updated PR.

czyzm commented 7 years ago
:lgtm:

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

Comments from Reviewable