Closed lhernanz closed 6 years ago
Please add new options in both versions of flash (darwin and linux) so that the docs are the same for both
As of today, both scripts are sharing 90% of the code. It is going to be very painful to keep them fully aligned and asking to replicate all the new features in the two scripts is going to be time consuming and error-prone.
I would suggest that we try to merge the scripts in order to support both OSs from a single script. I could try to create a first version but I do not have a darwin based system at hand, so I would need some help with testing the whole thing. Please, tell me if you would be interested in exploring such a thing
Of course, maybe you have discarded to do this in the past for a very good reason but, from a quick view at the code, it should be doable.
This feature is Linux only as Mac cannot mount ext4 by default to change Raspbian /etc/hostname file.
Merging both Linux + Mac scripts into one sounds good. I remember we got a first PR for Linux in a different script and so we went this way to update them independently and try to keep them in sync after a while.
Hi @StefanScherer, I guess that your comment is related to the other issue that I have opened (#108) about adding Raspbian support, because there is nothing Linux specific in this PR. In that case, you are right, the Raspbian based solution would be Linux only. You need to tell me which way you prefer to go. I could try to make a first attempt at the common script but only if you are interested. And you need to tell me if you would be willing to keep the Raspbian support. Thanks Regards
Hi @lhernanz, Yes I've mixed the two issues responding from mobile phone, sorry ;-) Refactoring the two scripts into one still seems very useful to me, I can help testing the PR on a Mac (maybe we can do some bats test or something similar on Travis as they also provide Mac and Linux builders :-) ) I would also keep the Raspbian support for Linux. Cheers, Stefan
Hi @lhernanz I'm currently adding BATS tests to have some tests running for upcoming changes and pull requests. See work in progress #111. At least the Linux script can be tested in a Docker container eg. https://github.com/hypriot/flash/pull/111/files#diff-573ac96d453a40199dbc09ada146c42bR4 I'll improve these tests and merge #111 and afterwards we can have a look how to merge Darwin and Linux into one script.
Hi @StefanScherer, this is a great idea before a refactoring! Just ping me when you think the test are stable enough and I will use them to validate my changes. This is the first time that I have seen BATS but, from the instructions, probably it would be a good idea that you would add the bats main project and the helper libraries as submodules of this project. In this fashion, you would download all the code you need to run the test locally. Now, you need to go for a manual installation of BATS and you need to rely on the relative paths that you have used.
@lhernanz just in time. I've finished the macOS port of the BATS integration tests and they also work on TravisCI. Now we get real feedback with some real script calls and checks. See #113 for a screenshot of both CircleCI and TravisCI. Years ago I thought this is impossible to test automatically.
I think I have to add some docs about running the tests. I had a look how to install all the tools and thought Docker would be the best fit. Therefore the Linux integration tests run in a Docker container, and a Makefile wraps the commands.
Run shellcheck on both Darwin/flash and Linux/flash scripts:
make shellcheck
Run integration tests with the Linux/flash script:
make test
You can push to pull requests and trigger builds on Travis and Circle and get feedback and logs in the GitHub UI.
The dependency hell is hidden both in package.json
and the Dockerfile
.
I use the npm modules of bats and bats-mock etc. as it simplifies the installation without git submodule. Then Docker is the whole wrapper for all the other tools needed to run flash in a isolated environment. Only docker build
and docker run
is needed which is then wrapped in a Makefile.
I just updated the feature and cleaned-up the merge request. I will try to test it with the new system, although I have already identify a permission issue in the docker file, as the container tries to write in the current directory where it does not have permissions.
Thanks. I wondered why CI was green. I forgot to enable forked builds in Circle. Now test 16 seems to hang in the security prompt and times out: https://circleci.com/gh/hypriot/flash/39?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link
I think running flash
without any parameters still should show the usage as no hostname etc is set and it will do nothing.
LGTM
When playing with different configurations, sometimes it is useful not to have to re-write the image every time. This not only saves time but also some unneeded writes to the SD card. Therefore, I propose to add an option to configure/reconfigure a pre-existing image.