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

build(docker): #57 Build from recent image #59

Merged
merged 8 commits into from
Feb 27, 2024

Conversation

AlexAxthelm
Copy link
Contributor

Build from a recent image from the public registry for workflow.transition.monitor. While these images don't include the pacta-data needed to prepare indices, the current README instructions include a requirement to externally mount PACTA-data.

Closes: #57

Build from a recent image from the public registry for
`workflow.transition.monitor`. While these images don't include the
pacta-data needed to prepare indices, the current README instructions
include a requirement to externally mount PACTA-data.

Closes: #57
@AlexAxthelm
Copy link
Contributor Author

long-term goal: use explicit pinning on SHA for stability

FROM ghcr.io/rmi-pacta/workflow.transition.monitor@sha256:4cb557ea3155f53a16253e85ed5a7986076c58eee335d685dd30183a038ef5b7

@cjyetman
Copy link
Member

maybe should update the README along with this?

The index preparation Dockerfile uses the `transitionmonitordockerregistry/rmi_pacta` docker image as a base image. Pulling this image requires access to the Azure docker registry `transitionmonitordockerregistry`.
You can log-in to this registry by calling:
``` bash
az acr login --name transitionmonitordockerregistry
```

Copy link
Member

@jdhoffa jdhoffa left a comment

Choose a reason for hiding this comment

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

Agreed, please update README accordingly

@cjyetman
Copy link
Member

verified that this works once #62 is included

@AlexAxthelm AlexAxthelm requested a review from jdhoffa February 26, 2024 17:30
README.md Outdated Show resolved Hide resolved
Copy link
Member

@cjyetman cjyetman left a comment

Choose a reason for hiding this comment

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

see minor spelling suggestions

cjyetman
cjyetman previously approved these changes Feb 26, 2024
@AlexAxthelm
Copy link
Contributor Author

Note, we can't get around the install requirement entirely, since pacta.data.scraping needs to be installed, but here's the relevant output from a build I just started

#8 55.06
#8 55.11 → Will install 14 packages.
#8 55.26 → All 15 packages (0 B) are cached.
#8 55.28 + askpass               1.2.0
#8 55.28 + curl                  5.1.0       + ✔ libcurl4-openssl-dev, ✔ libssl-dev
#8 55.28 + httr                  1.4.7
#8 55.28 + httr2                 0.2.3
#8 55.28 + logger                0.2.2
#8 55.28 + lubridate             1.9.3
#8 55.28 + openssl               2.1.1       + ✔ libssl-dev
#8 55.28 + pacta.data.scraping   0.1.0.9000 [bld][cmp] (GitHub: 91302af)
#8 55.28 + rvest                 1.0.3
#8 55.28 + selectr               0.4-2
#8 55.28 + sys                   3.4.2
#8 55.28 + timechange            0.2.0
#8 55.28 + V8                    4.4.0       + ✖ libnode-dev
#8 55.28 + xml2                  1.3.5       + ✖ libxml2-dev
#8 55.35 → Will install 2 system packages:
#8 55.37 + libnode-dev  - V8
#8 55.37 + libxml2-dev  - xml2

and we can see that. it's not trying to reinstall any of the other pacta packages.

@AlexAxthelm
Copy link
Contributor Author

Would it be prudent to fail the build if (pacta) dependencies are out of date?

@cjyetman
Copy link
Member

pacta.data.scraping is fine to install, it's just the packages already in the TM image that we should avoid overwriting.

Dockerfile Outdated Show resolved Hide resolved
Copy link
Member

@jdhoffa jdhoffa left a comment

Choose a reason for hiding this comment

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

I am trusting @cjyetman's review and rubber stamping.
I did not review this myself.

@AlexAxthelm AlexAxthelm merged commit fb550b1 into main Feb 27, 2024
@AlexAxthelm AlexAxthelm deleted the build/57-update-base-image branch February 27, 2024 12:19
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.

Point to recent build from worklfow.transition.monitor as base image
3 participants