-
Notifications
You must be signed in to change notification settings - Fork 226
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ci: schedule a Docker deployment test #4133
Conversation
a5071a3
to
2475cca
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The shell stuff looks okay. The yaml doesn't set off alarms. You might want another pair of eyes on the JS.
esac | ||
|
||
TERRAFORM_RELEASE=terraform_${TERRAFORM_VERSION}_${TERRAFORM_OS}_${TERRAFORM_ARCH} | ||
curl https://releases.hashicorp.com/terraform/${TERRAFORM_VERSION}/${TERRAFORM_RELEASE}.zip > terraform.zip && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure of the context in which this script is run, but double-check that it's okay to create potentially large files in whatever the current directory is. unzip
can't extract from stdin? Lame!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to use a temporary file with mktemp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Glossed for bash style. I highly recommend shellcheck.
@@ -0,0 +1,59 @@ | |||
#! /bin/bash | |||
# Get a deployment.json for a 2-node docker setup. | |||
set -e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommending set -ueo pipefail
as the standard utterance.
"PROVIDER_NEXT_INDEX": { | ||
"docker": 1 | ||
}, | ||
"NETWORK_NAME": "$NETWORK_NAME" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I distrust interpolation here on principle. If there's any chance you can rely on jq, I like the pattern of jq --arg NETWORK_NAME "$NETWORK_NAME" -n <<EOF
. If you can rely on node instead...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duly Noded.
esac | ||
|
||
TERRAFORM_RELEASE=terraform_${TERRAFORM_VERSION}_${TERRAFORM_OS}_${TERRAFORM_ARCH} | ||
curl https://releases.hashicorp.com/terraform/${TERRAFORM_VERSION}/${TERRAFORM_RELEASE}.zip > terraform.zip && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shellcheck would tell you to quote this so that it means the same thing if a field separator snuck into one of the variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I installed the Shellcheck VSCode plugin. Very nice!
# Install Ansible. | ||
echo 'deb http://ppa.launchpad.net/ansible/ansible/ubuntu trusty main' >> /etc/apt/sources.list && \ | ||
apt-key adv --keyserver keyserver.ubuntu.com --recv-keys 93C4A3FD7BB9C367 && \ | ||
apt-get update --allow-releaseinfo-change -y && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The \ is superfluous since the line ends with an operator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The JavaScript looks good to me.
// explanation. | ||
// eslint-disable-next-line @endo/no-polymorphic-call | ||
- console.log(`Removing ${subPath}`); | ||
+ console.warn(`Removing ${subPath}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this. I should prioritize fixing this generally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I'm not sure we should patch this out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The patch is an expedient to prevent actual failures when stdout is captured. When we upgrade SES and it fails to apply, we'll get a warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it fail to apply tho? The fix will not touch these console statements.
Why would stdout cause failures?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it fail to apply tho? The fix will not touch these console statements.
patch-packages
tracks the version number of the package you're patching. It will warn when the version doesn't match the one named by the patches/*+VERSION.patch
file.
Why would stdout cause failures?
./my-ses-program-running-on-node16 > ./critical-file.txt
Oops, now your critical-file.txt
starts with:
Removing intrinsics.Object.hasOwn
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh you're right, these should go to stderr
instead of stdout
! For some reason I misread the patch, sorry.
@kriskowal Any reason these are console.log
instead of console.warn
in the first place?
@@ -323,8 +326,7 @@ const doInit = ({ | |||
}); | |||
config.NETWORK_NAME = overrideNetworkName; | |||
|
|||
// eslint-disable-next-line no-constant-condition | |||
while (true) { | |||
while (!noninteractive) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I presume this halts!
@@ -34,7 +34,7 @@ jobs: | |||
# 'yarn install' must be done at the top level, to build all the | |||
# cross-package symlinks | |||
- name: yarn install | |||
run: yarn install | |||
run: yarn install --frozen-lockfile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
// explanation. | ||
// eslint-disable-next-line @endo/no-polymorphic-call | ||
- console.log(`Removing ${subPath}`); | ||
+ console.warn(`Removing ${subPath}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I'm not sure we should patch this out
2475cca
to
aa551d6
Compare
"$thisdir/setup.sh" init --noninteractive | ||
|
||
# Go ahead and bootstrap. | ||
exec "$thisdir/setup.sh" bootstrap ${1+"$@"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to exec here?
Should the test be nice and cleanly stop the bootstrapped chain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those are reasonable improvements, but not necessary for the Github Actions test, so I didn't do them.
refs: #3444
Description
Have CI bootstrap a network with the
packages/deployment
machinery. This should help detect both obvious multinode nondeterminisms as well as test out our deployment tooling.Security Considerations
Documentation Considerations
Testing Considerations
This PR produces the current known-failure at https://github.com/Agoric/agoric-sdk/runs/4361816825#step:7:2272