refactor: clarify upgrade builds #8259
Conversation
| @@ -32,6 +32,13 @@ if ! test -f "$HOME/.agoric/runActions-${THIS_NAME}"; then | |||
| touch "$HOME/.agoric/runActions-${THIS_NAME}" | |||
| fi | |||
|
|
|||
| #region alkjdf | |||
There was a problem hiding this comment.
didn't mean to push this. disregard
(draft but I'll clean up before real review)
There was a problem hiding this comment.
This is probably the reason tests are failing. Can you please remove it... I prefer not to be pinged for review of something with failing tests.
There was a problem hiding this comment.
apologies. I marked it as Draft to not claim it was ready to merge. I was hoping you could speak to the questions in the comments.
| @@ -9,96 +13,111 @@ RUN echo "${BOOTSTRAP_MODE}" | |||
| RUN mkdir -p /usr/src/agoric-sdk/upgrade-test-scripts | |||
| WORKDIR /usr/src/agoric-sdk/ | |||
| COPY ./start_ag0.sh ./upgrade-test-scripts/ | |||
| COPY ./bash_entrypoint.sh ./env_setup.sh ./start_to_to.sh ./upgrade-test-scripts/ | |||
| COPY ./bash_entrypoint.sh ./env_setup.sh ./start_to_to.sh ./package.json ./*.mjs ./upgrade-test-scripts/ | |||
There was a problem hiding this comment.
Looks like just to make all the different targets consistent. However, I disagree with this... the copy should be absolutely minimal for each target, or the build cache is invalidated more often than it should be!
| COPY ./${THIS_NAME} ./upgrade-test-scripts/${THIS_NAME}/ | ||
| COPY --from=agoric-upgrade-7-2 /root/.agoric /root/.agoric |
There was a problem hiding this comment.
AFAIK, this very much needs to be copied as it's the state of ag0/agd
There was a problem hiding this comment.
this very much needs to be copied
It is copied on green line 28 (just a moved line, not a deletion), but this threw me for a loop as well.
It looks like @turadg was trying to reorder each of these sections so that they matched one another.
| @@ -1,5 +1,9 @@ | |||
| # Defaults | |||
| # ??? why isn't this called SDK_IMAGE? | |||
There was a problem hiding this comment.
Because they're all SDK images. This is the final destination image for the entire chain of upgrades.
There was a problem hiding this comment.
FWIW the name wasn't intuitive for me either, at first I struggled with the DEST meaning (also used in shell scripts to signal steps that changed meaning over time). This is a sign to me that it could be called something better. All this to say that my best answer is...inertia 😅
| #this is agoric-upgrade-8-1 aka pismoB | ||
| FROM ghcr.io/agoric/agoric-sdk:30 as agoric-upgrade-8-1 | ||
| # ??? what is UPGRADE_INFO_9 for? |
There was a problem hiding this comment.
Each UPGRADE_INFO_${N} can be set via a Docker --build-arg=UPGRADE_INFO_${N}=<contents> to add an --upgrade-info=<contents> flag on the upgrade proposal for agoric-upgrade-${N}. It's a pattern that makes every software-upgrade proposal somewhat parameterizable.
| COPY ./${THIS_NAME} ./upgrade-test-scripts/${THIS_NAME}/ | ||
| COPY --from=agoric-upgrade-7-2 /root/.agoric /root/.agoric |
There was a problem hiding this comment.
this very much needs to be copied
It is copied on green line 28 (just a moved line, not a deletion), but this threw me for a loop as well.
It looks like @turadg was trying to reorder each of these sections so that they matched one another.
| @@ -9,96 +13,111 @@ RUN echo "${BOOTSTRAP_MODE}" | |||
| RUN mkdir -p /usr/src/agoric-sdk/upgrade-test-scripts | |||
| WORKDIR /usr/src/agoric-sdk/ | |||
| COPY ./start_ag0.sh ./upgrade-test-scripts/ | |||
| COPY ./bash_entrypoint.sh ./env_setup.sh ./start_to_to.sh ./upgrade-test-scripts/ | |||
| COPY ./bash_entrypoint.sh ./env_setup.sh ./start_to_to.sh ./package.json ./*.mjs ./upgrade-test-scripts/ | |||
There was a problem hiding this comment.
Looks like just to make all the different targets consistent. However, I disagree with this... the copy should be absolutely minimal for each target, or the build cache is invalidated more often than it should be!
| @@ -32,6 +32,13 @@ if ! test -f "$HOME/.agoric/runActions-${THIS_NAME}"; then | |||
| touch "$HOME/.agoric/runActions-${THIS_NAME}" | |||
| fi | |||
|
|
|||
| #region alkjdf | |||
There was a problem hiding this comment.
This is probably the reason tests are failing. Can you please remove it... I prefer not to be pinged for review of something with failing tests.
turadg
force-pushed
the
ta/upgrade-builds
branch
from
14d05b3
to
b9b8625
Compare
| ## this is agoric-upgrade-8 aka pismoA | ||
| FROM ghcr.io/agoric/agoric-sdk:29 as agoric-upgrade-8 | ||
| ARG BOOTSTRAP_MODE | ||
| # ??? why doesn't this set UPGRADE_TO ? |
There was a problem hiding this comment.
The upgrade from pismoA to pismoB was a hot fix (without a governance proposal), so it doesn't use any tx gov software-update.
Description
I had some difficulty making sense of the Dockerfile in #8254
This reorganizes a bit for more clarity. I also embedded a few questions I'd like to have answered in comments in the file
Security Considerations
Scaling Considerations
Documentation Considerations
Testing Considerations
Upgrade Considerations