Ensure the build check fails if there are new untracked files #77
Conversation
|
After investigation, I am not going to check in the |
brrygrdn
force-pushed
the
brrygrdn/update-check-diff-script
branch
3 times, most recently
from
eba18cd
to
c586bb1
Compare
| @@ -3,6 +3,10 @@ output/output.json | |||
| repo | |||
| tmp/ | |||
|
|
|||
| # Ignore optional native extensions for SSH | |||
There was a problem hiding this comment.
Thanks for the context on this 😄
| @@ -1,5 +1,7 @@ | |||
| #!/bin/bash | |||
|
|
|||
| # Make sure we notice any untracked files generated by the build | |||
| git add --intent-to-add . | |||
There was a problem hiding this comment.
This creeps outside the scope of the change you're introducing, but is related:
What do you think about replacing --quiet with --exit-code (on the line below this one)? My thinking is that we'd still trigger a build failure if the contents are different, but we'd be able to inspect the diff.
There was a problem hiding this comment.
That's a good call, I've modified the script to use --quiet, but in the event of a failure, output the full diff on line 8 (not shown in diff).
Not being able to see the diff was getting in my way a lot 😂
We pull in ssh2 via dockerode:
dockerode
↳ docker-modem
↳ ssh2
↳ cpu-features
Both it and `cpu-features` compile _optional_ native extensions using
OpenSSH and cpu-features (https://github.com/google/cpu_features) which
result in node-gyp generating two `.node` executables for these bindings
We have found that these bindings are very sensitive to the build env
which means that developer laptops, Actions and Codespaces result in
a diff in the `.node` files, something we seek to prevent in PRs in
order to detect cases where code changes are committed without actually
being applied to the `dist/` folder.
Since we do not actually SSH into any containers in our implementation,
let's just ignore these files in our distributed code rather than
make our build more convoluted/less portable.
brrygrdn
force-pushed
the
brrygrdn/update-check-diff-script
branch
from
c586bb1
to
32688a2
Compare
One gap in the current check run is that if a PR generates new files during a build which aren't checked as part of the PR, we ignore this.
This at least creates confusion as to why there is are untracked files locally when building on
main, but in the worst case could cause us to fail to check in something that might be required in certain runtime environments.Ignoring .node files generated by ssh2
We pull in ssh2 via dockerode:
Both it and
cpu-featurescompile optional native extensions usingOpenSSH and cpu-features (https://github.com/google/cpu_features) which
result in node-gyp generating two
.nodeexecutables for these bindingsWe have found that these bindings are very sensitive to the build env
which means that developer laptops, Actions and Codespaces result in
a diff in the
.nodefiles, something we seek to prevent in PRs inorder to detect cases where code changes are committed without actually
being applied to the
dist/folder.Since we do not actually SSH into any containers in our implementation,
let's just ignore these files in our distributed code rather than
make our build more convoluted/less portable.
Why not just pin OpenSSH?
That was my first thought, but both Codespaces and Actions appear to be using the same base image of Ubuntu and OpenSSH/OpenSSL:
I didn't want to get much further into the weeds of checking which developer lib versions where on both as the action currently works without these files being distributed, so they aren't involved in code paths we actually call.