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

Keep dockerfiles up to date #919

Merged
merged 3 commits into from
Dec 7, 2022
Merged

Keep dockerfiles up to date #919

merged 3 commits into from
Dec 7, 2022

Conversation

janisz
Copy link
Contributor

@janisz janisz commented Nov 30, 2022

Description

Monitor docker images and update them with @dependabot

Refs: dependabot/feedback#145

@janisz janisz requested a review from a team as a code owner November 30, 2022 08:53
@janisz janisz requested a review from Molter73 November 30, 2022 08:53
Copy link
Collaborator

@Molter73 Molter73 left a comment

Choose a reason for hiding this comment

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

Thanks for this PR @janisz! I left a few comments on some directories we probably don't want to have dependabot auto-update images.

The ones that don't have comments are mostly for CI and internal tooling, so we could have them auto-updated but I don't think it is 100% needed (and might lead to some unexpected errors in our tests, or might not 🤷🏻‍♂️ ). @stackrox/collector-team what do you guys think? Do we want to add auto-updating CI images?

.github/dependabot.yaml Outdated Show resolved Hide resolved
.github/dependabot.yaml Outdated Show resolved Hide resolved
Comment on lines +16 to +17
- package-ecosystem: 'docker'
directory: collector/container/
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have 2 dockerfiles here:

Dockerfile.template uses ARGs to set the image being used as base and I'm not sure dependabot will pick up on it.
Dockefile.ubi is not being used right now, so updating it would not really have any effect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

That link talks about dependabot finding files with names other than Dockerfile, I'm more concerned about the actual content of the dockerfile:

ARG BASE_REGISTRY=registry.access.redhat.com
ARG BASE_IMAGE=ubi8/ubi-minimal
ARG BASE_TAG=8.6
FROM ${BASE_REGISTRY}/${BASE_IMAGE}:${BASE_TAG}

Copy link
Contributor Author

@janisz janisz Nov 30, 2022

Choose a reason for hiding this comment

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

I give it a test and you are right, args are not updated https://github.com/janisz/collector/pulls

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If Dockefile.ubi is not being used right now, why not deleting it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry for the delayed response. Basically, that image was used as a mirror to the downstream release, it fell out of use because OSCI is similar enough to downstream and we didn't bother to re-implement it. But now we are considering going to GHA, so it might make sense to go back to building this image.

.github/dependabot.yaml Outdated Show resolved Hide resolved
.github/dependabot.yaml Outdated Show resolved Hide resolved
.github/dependabot.yaml Outdated Show resolved Hide resolved
@ghost
Copy link

ghost commented Nov 30, 2022

COLLECTOR_TAG=3.12.x-30-gaccd7db8e3
COLLECTOR_BUILDER_TAG=cache

Results for Performance Benchmarks on build #

Kernel Method Without Collector Time (secs) With Collector Time (secs) Baseline median (secs) Collector median (secs) PValue
rhel.rhel-7 ebpf 235.196 287.094 149.14 187.29 🟢
rhel.rhel-7 module 97.027 114.752 165.94 190.59 🟢
rhel.rhel-8 ebpf 121.708 127.309 191.09 202.03 🟢
rhel.rhel-8 module 120.887 135.889 170.31 191.76 🟢
ubuntu-os.ubuntu-1804-lts ebpf 170.432 147.438 187.49 208.44 🔴
ubuntu-os.ubuntu-1804-lts module 149.364 159.039 187.65 219.87 🟢
ubuntu-os.ubuntu-2004-lts ebpf 214.357 213.185 234.53 234.3 🟢
ubuntu-os.ubuntu-2004-lts module 171.681 205.702 224.94 235.04 🟢
ubuntu-os.ubuntu-2204-lts ebpf 189.099 170.844 215.12 227.21 🟢
ubuntu-os.ubuntu-2204-lts module 181.347 179.133 218.58 241.19 🟢

@janisz janisz requested a review from Molter73 November 30, 2022 11:49
@janisz
Copy link
Contributor Author

janisz commented Dec 5, 2022

@Molter73 PTAL

Copy link
Collaborator

@Molter73 Molter73 left a comment

Choose a reason for hiding this comment

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

/lgtm

@janisz janisz merged commit 5d1a8b7 into master Dec 7, 2022
@janisz janisz deleted the Keep-dockerfiles-up-to-date branch December 7, 2022 11:05
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