Skip to content
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

Bring back docker image #1211

Merged
merged 9 commits into from
Jun 17, 2022
Merged

Bring back docker image #1211

merged 9 commits into from
Jun 17, 2022

Conversation

david-crespo
Copy link
Contributor

@david-crespo david-crespo commented Jun 15, 2022

Turns out the CLI needs it for testing, plus @karencfv will also be using it for testing Terraform.


Here's the diff between this branch and the commit before the one where I deleted the Dockerfile: a09934d...undelete-docker. The only difference from the original versions of these files (the Dockerfile and the GH workflow file) is that I added a apt-get install line before cargo build.

The natural thing would be to run tools/install_prerequesites.sh there instead, but I couldn't get that to work for a couple of reasons. First, it doesn't like the sudo commands — sudo is not available by default. Getting rid of the sudos made it get farther (CI run) but then it failed on something else:

#12 20.15 ERROR: cockroach seems installed, but was not found in PATH. Please add it.
#12 20.15 cockroach should have been installed to '/usr/src/omicron/out/cockroachdb/bin'
#12 20.15 ERROR: clickhouse seems installed, but was not found in PATH. Please add it.
#12 20.15 clickhouse should have been installed to '/usr/src/omicron/out/clickhouse'

Not sure what to do about that.

In any case, to prove it can work, I have instead taken install_prerequisites.sh back out of the Dockerfile and instead manually copied in the packages it installs into an explicit apt-get install in the Dockerfile. That works, though it is of course brittle because it doesn't automatically bring in packages as they're added to the prereqs script. There are also probably more packages being installed than we actually need.

Dockerfile Outdated
Comment on lines 19 to 28
RUN apt-get update && apt-get install -y \
libpq-dev \
pkg-config \
xmlsec1 \
libxmlsec1-dev \
libxmlsec1-openssl \
libclang-dev \
libsqlite3-dev \
--no-install-recommends \
&& rm -rf /var/lib/apt/lists/*
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding this is the only change from the old Dockerfile

@karencfv
Copy link
Contributor

Thanks for bringing this back! I think adding those two to $PATH might do the trick.

Can you try adding ENV PATH="/usr/src/omicron/out/cockroachdb/bin:/usr/src/omicron/out/clickhouse:${PATH}" or ENV PATH="/usr/src/omicron/out/cockroachdb/bin:/usr/src/omicron/out/clickhouse:$PATH" (I never remember if it's with or without the curly braces 😄 )

Copy link
Contributor

@karencfv karencfv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙇‍♀️

password: ${{ secrets.GITHUB_TOKEN }}
- name: Extract branch name
shell: bash
run: echo "##[set-output name=branch;]$(echo ${GITHUB_REF_NAME//\//-})"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was changed to fix a bug where it was tagging these images as main

# sudo and path thing are only needed to get prereqs script to run
ENV PATH=/usr/src/omicron/out/cockroachdb/bin:/usr/src/omicron/out/clickhouse:${PATH}
RUN apt-get update && apt-get install -y sudo --no-install-recommends && rm -rf /var/lib/apt/lists/*
RUN tools/install_prerequisites.sh -y
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was added vs. the original Dockerfile

@david-crespo david-crespo merged commit 7020b0f into main Jun 17, 2022
@david-crespo david-crespo deleted the undelete-docker branch June 17, 2022 04:37
leftwo added a commit that referenced this pull request Mar 29, 2024
Propolis changes:
Add `IntrPin::import_state` and migrate LPC UART pin states (#669)
Attempt to set WCE for raw file backends
Fix clippy/lint nits for rust 1.77.0

Crucible changes:
Correctly (and robustly) count bytes (#1237)
test-replay.sh fix name of DTrace script (#1235)
BlockReq -> BlockOp (#1234)
Simplify `BlockReq` (#1218)
DTrace, cmon, cleanup, retry downstairs connections at 10 seconds.
(#1231)
Remove `MAX_ACTIVE_COUNT` flow control system (#1217)

Crucible changes that were in Omicron but not in Propolis before this commit.
Return *410 Gone* if volume is inactive (#1232)
Update Rust crate opentelemetry to 0.22.0 (#1224)
Update Rust crate base64 to 0.22.0 (#1222)
Update Rust crate async-recursion to 1.1.0 (#1221)
Minor cleanups to extent implementations (#1230)
Update Rust crate http to 0.2.12 (#1220)
Update Rust crate reedline to 0.30.0 (#1227)
Update Rust crate rayon to 1.9.0 (#1226)
Update Rust crate nix to 0.28 (#1223)
Update Rust crate async-trait to 0.1.78 (#1219)
Various buffer optimizations (#1211)
Add low-level test for message encoding (#1214)
Don't let df failures ruin the buildomat tests (#1213)
Activate the NBD server's psuedo file (#1209)

---------

Co-authored-by: Alan Hanson <alan@oxide.computer>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants