qa: add build ci #23
qa: add build ci #23
Conversation
|
I'm not entirely sure why this doesn't get triggered on this PR itself, so I'm investigating that. In the meantime, find a test run of this commit here: https://github.com/patricklodder/docker-dogecoin/actions/runs/1536909449 |
There was a problem hiding this comment.
works for me, can sus out the testing in another pr?
https://github.com/xanimo/docker/actions/runs/1537078369
patricklodder
force-pushed
the
qa/ci-for-pulls
branch
2 times, most recently
from
6aedfa6 to
13b18c2
Compare
|
Fixed 3 issues:
|
patricklodder
force-pushed
the
qa/ci-for-pulls
branch
2 times, most recently
from
0b4415a to
ac8fd52
Compare
|
Linted with semistandard to standardize the js as much as possible but not run into readability issues because of all the piping going on (no semicolons will make it unreadable, so instead we just enforce semicolons). |
There was a problem hiding this comment.
Quick review, I need to take time to dive into it and to see what's going on. I didn't do a lot of javascript and javascript nested calls within GitHub Action are fucking my brain...
Does the output of the genmatrix.js is a json with versions as keys and distros ? Variants are distros ? If you have an output example to share, it would be appreciated.
I will dive deeper in the PR in the following days, to be clear with the behavior.
Yes, It's in GH Actions format for build matrices (
{
"include": [
{
"version": "1.14.5",
"variant": "bullseye",
"platform": "linux/amd64",
},
...
]
}I kept |
|
@AbcSxyZ after reading about headaches, I'll put this back to draft and instead of minimizing code, I'll write out 2 classes and make it more structured and documented so that it's easier to read. ETA ~ 3h. |
patricklodder
force-pushed
the
qa/ci-for-pulls
branch
from
fb81965 to
056815d
Compare
patricklodder
force-pushed
the
qa/ci-for-pulls
branch
2 times, most recently
from
7c0552c to
4aa888b
Compare
|
@AbcSxyZ let me know is this is easier to read. I haven't squashed the changes yet in case there's more. |
|
I try to have an idea of the testing strategy to understand where we are going. I think it can depend on #16 and a potential script to generate releases/updates. I'm thinking about it because if we use templates, maybe the goal would be to be consistent across all versions, edit the template and propagate changes. I think it would be the ideal scenario, and in this case you will be sure every Dockerfiles/entrypoint will change simultaneously on updates. How do you consider using a script to generate new releases & manage updates ?
Do you have any issue example ? Because if we go with If we stay with those distros with 4 type of binaries, I don't see the problem rn. Improvement suggestionI was wondering how to test efficiently architectures. With this build test, if you give a fixed value for Maybe with the shasums file, we could grep hash by architecture ( It's just an idea, not sure if it worth it and if we can't rely on the current Dockerfile for this, may ensure more accurately that with have downloaded the right file (at a potential cost of simplicity/readability). |
This is a Continuous Integration (CI) framework proposal. So what it enables is end-to-end integration tests, on PRs. #16 is a Continuous Deployment (CD) proposal, that runs on push. PRs precede push, and if you automate on push you better make sure that whatever you push is 100% correct, or else you'll push bugs. So for #16 to be feasible, we need to be sure that no PRs get merged that contain errors. This means that:
Note that none of this replaces any functional testing that you are proposing under #25.
That imho only makes the need for a good CI greater, not smaller. The more you automate the better your checks must be. For now, feel free to write a bot that does PRs.
Incorrect, because you're only considering the current availability of only the docker bases. You need to look at 2 more variables at least: Dogecoin Core binary availability and time. Currently there are no Dogecoin Core binaries for musl-based linux. Looking at the process, it will be pretty hard to do musl compilation for different architectures all at once and I don't know how desirable that will be from a Dogecoin Core perspective. Say I get a working musl gitian descriptor for 1.21.0, then 1.14.5 still doesn't have it, but we will need to support that for a while. So over time, things will change. Will be the same for any other added architecture, or OS (think native docker for windows or darwin.)
Build the architecture, run the tests. Many images have a test stage, we can add this. That would have the upside that we can just build the test target(s) in buildx, but it will have a downside that these targets will get built when doing
I have the feeling that you are proposing this because you don't like the level of automation I'm adding (or the As for the manifest I mentioned that I want instead of the This is really just step 1 of many to get production quality images. |
|
Multiples interrogations are coming to my mind due to multiple factors being involved, I'm definitively unsure about the strategy to adopt. Need of cross architecture build for testing ?At first, I'm not convinced it's useful to test cross architecture build, and I'm wondering if testing the build of single arch isn't enough to make sure it builds for all architectures ? I'm thinking about it for various reasons. We do not edit the Dockerfile related to architectures, it's a "built-in" features with Can you share the Docker repository where you get the design idea, please. By looking at official images, they seem to perform CI on push for all versions, but do not test cross-architecture build (here for Github Action CI result for php). My previous idea could make sense regarding their testing mechanism. Nor they mention information related to architectures in their versions.json. They use Docker Hub to pull image and not GA, it provides some testing tools too, maybe another layer of tests is done there but I doubt about it. Actually, we do not test that binaries are working on their architectures, and I suppose/assume it's done previously by Dogecoin Core tests. Maybe for the docker setup, we have more to take care about delivering the right executable per architecture rather than having executables running on those architectures. That was the idea behind my improvement suggestion, which can be an extra security check but does not in any way replace what we're implementing here. Could it be enough to test the build for a single architecture and run tests from #25 ? Alpine strategy (and other OS)I don't understand for Alpine what we could have. Will we have a binary released per architecture (with new arch added slowly), or would we build it from source (which is against gitian process as you may mention) ? I'm wondering if we are not over-engineering for now. Definitively agree we should keep in mind extension for alpine or whatever when it's appropriate, but for now I feel like we want to anticipate a scenario where we would have different architectures between different version for some executables we don't even have. Let's stay simple ? Write extensible code but do not add code for something who may come. Let's get a musl executable before implementing its Dockerfile. Update strategy based on templateIf we choose to follow or not this path to work from a template to propagate changes, it could have important influence on the design from my pov. As I mentioned, some check would be avoided because if you update from a template, you're sure you would need to update all generated Dockerfiles. In some ways you do not work on edited files as you do here, but on all available versions tracked by an intermediary file ( Don't you think we should already consider moving to this template based strategy, to facilitate new releases, but also to facilitate maintaining (of various versions) ? |
No. I make a point about this because besides that Dogecoin Core itself does not have quality gates defined for ARM-based releases other than "it compiles" right now (and this should be fixed there, but I'm still in the process of removing 1.14 technical debt - it's a seemingly endless work in progress), Debian maintains a branch per architecture for the base image. That means we cannot (and should not) trust that any given debian image tag for architecture A yields the same results as architecture B. I am confident that the Debian image maintainers do their utmost best to make it awesome, but do not ever forget that this software governs a lot of value. The principle that crypto is built upon - per Bitcoin's lead - is after all: don't trust, verify. The question is of course how much will we verify. Making sure that each image works... is in my opinion not a big deal. It just means putting some work into automation. I've already done a lot of the work, click the "files changed" tab above.
No, but it's a start. #25 doesn't implement any CI though. This framework could be used to automatically run the script from #25 cross-architecture.
The entire principle of continuous integration testing is built around learning about problems before you merge. Ignoring the fact that the architecture images are different can cause unforeseen issues. It's better to react to an issue when it's a PR, then when it's popping up in someone's production environment. The latter causes much stress and a costs a lot of time. The more inclusive the CI, the better we can prevent that.
CI should happen regardless of a pull or push publication. I think push can be more transparent, but I haven't studied Docker's publicly published push auditing in-depth. Maybe they have something better.
Yes. But it doesn't change anything. It basically removes the
For this repository, I think we must only release gitian-built binaries. Currently, one can create a user-managed Dockerfile for Alpine that self-compiles and the author takes full responsibility for any money lost while people are running it. It is imho not wise to do closed-process compilation from this organization. So yes, binaries must be released per gitian and yes, they will definitely not be for all architectures at once. I've seen the issues with the cross-compilation toolset for musl for x86_64 right now, and don't even want to think about ARM yet. One step at a time.
Depends. We're basically adding deployment to something that never has done that before. With that comes a LOT of responsibility. I think we should take that seriously. Question though: is all this just because you don't like the platform enumeration? The javascript? The fact that there is javascript? The fact that there is an extra file? All I hear is: let's not do it. Recent events in the Dogecoin ecosystem have lead to massive losses due to under engineering. Let's stay vigilant and not neglect verification that things work, because we're not a bank that can edit the books, or Ethereum for that matter. |
Not at all. Sure, I would appreciate being able to use python to manipulate files with GitHub action 🐍, but they do not seem to offer this possibility. I'm not afraid of JavaScript, I just need friendly code :) I think my concern is the balance between utility of tests vs resources consumption. It does not cost a lot of lines to launch a test for x4 architectures, but does it worth it and what are we testing ? Does the download/build of resources for versions * distros * architectures * pull request is not too big if we just verify that we can launch Actually, I wasn't even thinking about verifying Debian... I was thinking about verifying if executables worked, if executables are downloaded properly for different architectures, here it could make sense. As you mentioned, some tests are missing for arm, but it's not the place, and it should be done prior to this. We shouldn't run here the tests to ensure Dogecoin Core run on arm, so it's not what we're testing now. Going in this logic (I'm not sure about what verifying Debian could mean), that's every executable where you put 0 trust, you wouldn't assume that For now, regarding our current tests and other official images repos (could you link me the repo where you picked the idea ?), I don't see the benefit of doing the build of cross architectures for testing I don't know what, and only see the important consumption of resources. So what are we testing which require cross compilation ? |
I exploded 60 lines of lambdas into 150 lines of documented Builder pattern code... if you need more, tell me what. We already have a much more engineered version of what nodejs image does in a couple of lines... that I made for YOU.
How much resources does it consume then? Are you worried about the carbon footprint? Do I need to do it manually on each PR so that it costs renewable energy and my sweat only?
This really is not enough. Please point me to a CI that manages financial systems where checks go as far as checking if apt and wget works, that someone else packaged and tests... that gives zero meaning. The thing we want to test is if the image we propose to create with a new pull request works. And if it doesn't, we can fix it before merge. Currently, #25 only covers some functionality of entrypoint.py and doesn't test the Dockerfile other than that the entrypoint.py is copied there. Just to give you an example based on the first line in that file: an error in
That's exactly what I'm proposing. Assuming that it works because apt and wget works, is not the same as verifying.
Agreed. We should test that the binary that we've downloaded runs on the image we've created.
See my issue? The fact that wget, gpg, shasum and bash all work does not mean that it is safe to assume that the build is successful. That's what we need Continuous Integration for. And because each architecture has a different source (even if slightly) it is diligent to check them all if we can. And we can, because I coded that for us with this PR.
The images, end-to-end. Every single one. We have 4 products, so we test all 4. If we have 20, we test all 20. |
|
First, I didn't complain about current js code. If you did it only for me, go back to lambdas. If you did it to increase the readability even for non JavaScript dev in the goal of facilitating integration of people in the code, keep it. Don't do things specifically for ME because I'm too dummy, I just thought adding meaningful variable name and avoid too much nesting is appreciable.
C'mon... There is a difference between asking someone to do test by hand vs doing right tests and avoid use of huge amount of resources for potentially useless tests. I'm not against doing it, I just need a meaningful purpose.
So, how do we test the last step and not restrict ourselves to verify if wget & bash works ? For me, with the current PR, we test all steps without the last one. It's why I think it's a waste of resources. I do not know any Dockerfile who cross build to just verify if the cross build works. Reliability of image like mysql is critical, more than us, they probably don't avoid the test of cross build only to get a more spicy life. |
Of course, I guess I put that too hard - if I wouldn't agree, I wouldn't have coded it. To me, it's a now versus later thing. I'm not saying you're dummy, at all. I'm trying to accommodate you in the proposal of a CI framework so that we can build upon it sooner rather than later, because currently we are manually testing and unless any of the reviewers has a consistent test framework that is ran consistently, we may be missing things. I'm not saying we have missed things, but this type of automation will help.
Like I mentioned in the last bullet point of my first comment, I left the tests out to focus on the framework first. So of course the framework is not complete. All it does now is test the build. It would have found the following PR comments:
one out of 3 was architecture specific.
We need to test integration of the recipe for all resulting components. I'd at the very least want to run the container with the following params:
that would allow us to verify that we got the correct binary, and make sure that the entrypoint works end-to-end. Then, I think we must test
And then we have a minimal integration test. I don't think this is complete, but it would be a good start.
I fully disagree with this sentiment. MySQL is not a financial system so in case it would be used as a component in one, it simply passes on the test burden to the implementing system. I'm not sure that we can do that here. From my personal experience in designing and building financial systems, CI for these systems are much more complex than what I'm proposing here and have many more checks than we can possibly produce. You seem to ignore that A block not interacted with cannot be reversed, and we have time-critical features, like CLTV and I'm quite sure we'll introduce CSV as one of the next protocol enhancements. Imagine being a merchant and your software is broken because we didn't catch a bug in our CI, and oops, a timelock expired while you were down... That can translate into real money lost. Small mistakes can be disastrous, especially when you realize that institutions are using this software for custodial services. Some, as recently became apparent, even use the wallet function to store billions of DOGE. This means that by publishing a software, this use-case is enabled. So we better make really sure that whatever we publish is good. Until the birth of this repository, deployment of dogecoind was not touched upon, so we're doing something pretty novel (and risky) here by taking this out of user space. Yes, Dogecoin was made as a joke. But digressions aside, development of it has been a serious thing since development of 1.7. In any Dogecoin software published we have to be serious and precise, so that the joke can live on. |
There was a problem hiding this comment.
ACK with rebase + squash of 'fixup'.
https://github.com/xanimo/docker/runs/4454292033?check_suite_focus=true
Why don't we adopt what they do in ruimarinho/bitcoin-core and compare the output of which for us at least as of 1.14.3 appended daemon between core and version and use our RLS_VERSION like so: This would be placed in between our ENTRYPOINT and CMD lines. Would this satisfy your testing requirement of |
patricklodder
force-pushed
the
qa/ci-for-pulls
branch
3 times, most recently
from
f09c2ad to
9566c4c
Compare
|
Read the discussion above? |
There was a problem hiding this comment.
ACK. Tested here https://github.com/xanimo/docker/actions/runs/1556480408 and locally:
linux/amd64
version
----------------------
Running command: docker run --platform linux/amd64 -e VERSION=1 xanimo/1.14.5-dogecoin:ci
Running command: docker run --platform linux/amd64 xanimo/1.14.5-dogecoin:ci dogecoin-cli -?
Running command: docker run --platform linux/amd64 xanimo/1.14.5-dogecoin:ci dogecoin-tx -?
Tests successful
RESULTS: for xanimo/1.14.5-dogecoin:ci on linux/amd64
version: Success
Finished test suite with 1 successful tests and 0 failures
linux/arm64
version
----------------------
Running command: docker run --platform linux/arm64 -e VERSION=1 xanimo/1.14.5-dogecoin:ci-arm64
Running command: docker run --platform linux/arm64 xanimo/1.14.5-dogecoin:ci-arm64 dogecoin-cli -?
Running command: docker run --platform linux/arm64 xanimo/1.14.5-dogecoin:ci-arm64 dogecoin-tx -?
Tests successful
RESULTS: for xanimo/1.14.5-dogecoin:ci-arm64 on linux/arm64
version: Success
Finished test suite with 1 successful tests and 0 failures
linux/arm
version
----------------------
Running command: docker run --platform linux/arm -e VERSION=1 xanimo/1.14.5-dogecoin:ci-arm
Running command: docker run --platform linux/arm xanimo/1.14.5-dogecoin:ci-arm dogecoin-cli -?
Running command: docker run --platform linux/arm xanimo/1.14.5-dogecoin:ci-arm dogecoin-tx -?
Tests successful
RESULTS: for xanimo/1.14.5-dogecoin:ci-arm on linux/arm
version: Success
Finished test suite with 1 successful tests and 0 failures
linux/386
version
----------------------
Running command: docker run --platform linux/386 -e VERSION=1 xanimo/1.14.5-dogecoin:ci-386
Running command: docker run --platform linux/386 xanimo/1.14.5-dogecoin:ci-386 dogecoin-cli -?
Running command: docker run --platform linux/386 xanimo/1.14.5-dogecoin:ci-386 dogecoin-tx -?
Tests successful
RESULTS: for xanimo/1.14.5-dogecoin:ci-386 on linux/386
version: Success
Finished test suite with 1 successful tests and 0 failures
lint:
pylint ./tests --verbose
No config file found, using default configuration
------------------------------------
Your code has been rated at 10.00/10
There was a problem hiding this comment.
We maybe can do it in 2 steps, move the test framework inside a new PR and add the GA integration right after.
I comment here for now, I dive into the code it's a first review. It works fine, as I see, we will use qemu emulation finally.
The framework require some engineering to run (vs a framework where it's already done), it may require some time to appropriate it [doc needed], test failures do not have real debug information.
So, is it the "most awesome test framework you have ever seen written" ? Sadly ... I may break a dream ... but I don't think so 🙊.
I would love to see it, but it's not for today ❤️
Tests could be build on top of existing framework (this kind of test could have been implemented on top of #25, inside a container, using pytest for example), but I clearly agree of the importance to be consistent with Dogecoin Core test framework. Could we use a simpler test framework because needs with the Core are different, or is it better to stay with a design like the Core ? It's a matter of choice and it's up to you, I will follow the decision.
If we go this way, we clearly have to break #25 and change the design of the tests, otherwise we will have 2 concurrent testing mechanism.
Yesterday you said "For me, it would be NACK without some sort of tests, at least
Addressing this, but can we please make this subject to future improvement?
Way ahead of you! I already anticipated and answered your opinion per the second part of my original sentence 😂 - it was already self-deprecation, no need to deprecate me, I've already done it for you. Thank you for granting me the opportunity to improve though!
This will move closer to features offered by Core's framework when we write tests that interact with a running container. DockerRunner will then become stateful for starting and stopping containers. I could have simply made it a function call now and skip class instantiation, but this is anticipating the greater scheme of where we need to go. I removed a lot of methods from my draft class, to ease review now, but will introduce it when it's needed. Make your life easy by doing a step-by-step approach while planning for the future.
We will have more than 2 concurrent testing/auditing mechanisms; right now proposed:
Both are needed! I beg you to please not refactor #25 to match this because it has a completely different purpose: unit tests validate atomic function behavior against its technical design and need features like mocking and code-traceability, whereas integration tests validate the behavior of the entire product assembly against its functional design, and need execution log traceability. Integration test failures are inherently more difficult to troubleshoot, and I support your remarks that the trace is not as good as it can be, yet. But then, as I mentioned earlier, we will also need:
and then finally:
|
|
Fine to avoid a separate PR, it was potentially to speak about the handling of error message/assert errors + implication of the framework with #25. Tests are working, nothing is really blocking for the test framework. About the dynamic management of architecture through Idk when we would introduce manifest, maybe for the templating it could be useful, for qa at a moment, but I think this PR may be not the right place nor the right design. Actually, the introduction of a manifest will simplify genmatrix.js behavior. |
Alternatively we can hardcode it inside the genmatrix.js file in the interim, which I think is worse than this proposal, because changing that file indicates a CI change and will remove benefits of using resources sparingly; see this test pr CI run as an example of how the matrix generation optimizes the test as described in bullet point 3 of my original comment all the way up there... I'm okay with replacing it but I'd suggest doing it later, especially since we have more pressing issues right now, like getting the CI and linters up, so that PRs see QA and we have some assurances that we don't create semi-functional images and have to do a lot of error-prone manual testing when we think this is ready for a first release.
Doesn't have to be now, but we either do it before we release this and have a future-proof build in place, or do it before we are faced with a new version of either Debian or Dogecoin Core. I can tell you that unless larger CVEs get reported, current work on 1.14.6 will take some time due to the security enhancement requested yesterday.
I'm not able to guarantee simplification completely (do strife for it) but I agree that it will have to change, for sure. We probably need to add things to it for #15 too. |
|
Please let me know if the assertion improvement in version.py is good enough for now. If it is, I'll squash the fixups |
There was a problem hiding this comment.
ACK for me, test working locally and with GA. For the assert, it's sad to not use assert keyword, but I don't see how it could be better for now.
Similarly, you could have used the following, not sure if the result is exactly the same or slightly different:
text = f"Could not find version { self.options.version } in { first_line }"
assert re.match(self.version_expr, first_line) is None, textBut for clarity, I prefer your version.
This sentence is unclear for me "that file indicates a CI change and will remove benefits of using resources sparingly;". I still don't see the real utility of PLATFORM and in favor of hard coded values, but if you prefer to remove it later.
|
I'll squash now for final review on a clean commit set, because this has too many useless commits right now |
patricklodder
force-pushed
the
qa/ci-for-pulls
branch
from
ad8db65 to
11baed2
Compare
There was a problem hiding this comment.
ACK. Tested locally and on GA. Good catch on the docker-entrypoint.py -> entrypoint.py in .github/workflows/build-ci.yml @AbcSxyZ! And great job @patricklodder! Thank you both!
Adds a GH Actions CI for testing pull requests across all build targets.
Notes:
PLATFORMSto each version/platform pair (which is ugly) until we have settled on some sort of machine-readable manifest (I have a ton of thoughts about this, will create separate issue for it later.) Either way, different Dogecoin Core versions and different distros can support different platforms, so we need to document this somewhere...