Docker containerId - cgroupv1 & v2 compatible #1434
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1434 +/- ##
=======================================
Coverage 96.56% 96.56%
=======================================
Files 614 614
Lines 8814 8818 +4
=======================================
+ Hits 8511 8515 +4
Misses 303 303
Continue to review full report at Codecov.
|
|
Do we agree that this PR is no longer up to date after what we said on friday? |
|
Yes you are right, I need to push the part concerning cidfile |
|
@Pierre-Gilles @atrovato , I don't really understand why test fail on bluetooth here, any clue ? |
|
No idea for the bluetooth, I just restarted the build because sometimes the bluetooth fails for no reason (noble is not very stable) |
|
@VonOx See changes in patch here ;) |
|
Thank you @atrovato , but I'm not sure to understand how my test can have an impact on bluetooth test. ( on my view completly independant , in reality not 😦 ) |
| 'cat /proc/self/cgroup | grep -o -e "docker.*" | head -n 1 | sed -e "s/.scope//g;s/docker-//g;s!docker/!!g"', | ||
| ); | ||
| } else { | ||
| this.containerId = fs.readFile('/var/lib/gladysassistant/containerId', 'utf8'); |
There was a problem hiding this comment.
This won't work, you are using a callback (async) function, the result won't be there.
You need to use async/await.
Two possibilities:
- Promisify fs (for example with bluebird), because fs doesn't return promise by default.
- Use fs-extra, a lib we already use in Gladys: https://github.com/jprichardson/node-fs-extra
In Gladys 4 we don't use the callback pattern (because of the famous callback hell issue), and we rely only on async/await.
There was a problem hiding this comment.
@Pierre-Gilles what about const { promises: fs } = require("fs"); ?
There was a problem hiding this comment.
Indeed! I didn't know about this one, I learned something :D
You'll need to change the access too btw
| let fsError = false; | ||
| const fsMock = { | ||
| ...fs, | ||
| access: (file, options, callback) => { |
There was a problem hiding this comment.
You'll need to mock this as a async function
You can use fake.resolves() to do this (search in the codebase for fake.resolves and you will see some examples :)
There was a problem hiding this comment.
Hard to learn, is Docker test use the same
There was a problem hiding this comment.
You stubbed some functions globally in your test, so it affected other tests too. |
|
I improved this PR here: #1459 |
Rework on the getNetworkMode, more precisely getting the container Id:
So documentation and rpi image need to be updated too and cgroup "usage" deprecated