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.

Sign up for GitHub

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

Jump to bottom

konflux: add a dockerfile to must gather #1157

Merged
merged 1 commit into from
Jan 21, 2025
Merged

konflux: add a dockerfile to must gather #1157

merged 1 commit into from
Jan 21, 2025

Conversation

rbaturov
Copy link
Contributor

This Dockerfile will be used by konflux.

@rbaturov
Copy link
Contributor Author

@shajmakh @ffromani
We need to decide if we want to place this in the root dir, or create a .konflux dir to hold the dockerfiles used by konflux.

@rbaturov rbaturov force-pushed the must-gather-dockerfile-konflux branch from 4f18ae8 to 99c2cfe Compare January 20, 2025 13:36
shajmakh
shajmakh reviewed Jan 20, 2025
View reviewed changes
Copy link
Member

@shajmakh shajmakh 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.
/lgtm
this can be simplified more but starting from here is fine

Dockerfile.must-gather.konflux Outdated
Comment on lines 23 to 33
LABEL com.redhat.component="numaresources-must-gather-container" \
name="openshift4/numaresources-must-gather-rhel9" \
upstream-vcs-type="git" \
summary="numa resources data gathering image" \
io.openshift.expose-services="" \
io.openshift.tags="data,images" \
io.k8s.display-name="numaresources-must-gather" \
description="numa resources data gathering image." \
maintainer="[email protected]" \
io.openshift.maintainer.component="NUMA Resources Operator" \
io.openshift.maintainer.product="OpenShift Container Platform"
Copy link
Member

Choose a reason for hiding this comment

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

likely konflux won't need these, but as long as these doesn't fail the konflux flow we can clean up later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is required by konflux, or at least some of them.
I was having some failures in the default integration test (ECP), complaining that the build image missing some of these lables

Dockerfile.must-gather.konflux Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 20, 2025
@shajmakh
Copy link
Member

/hold
for @ffromani 's review

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 20, 2025
@rbaturov rbaturov force-pushed the must-gather-dockerfile-konflux branch from 99c2cfe to 21c1341 Compare January 20, 2025 14:35
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 20, 2025
ffromani
ffromani reviewed Jan 20, 2025
View reviewed changes
Copy link
Member

@ffromani ffromani left a comment

Choose a reason for hiding this comment

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

can we move the konflux docker files in (any) konflux-specific directory?

Dockerfile.must-gather.konflux Outdated Show resolved Hide resolved
Dockerfile.must-gather.konflux Outdated

FROM registry.redhat.io/rhel9-4-els/rhel-minimal:9.4

RUN microdnf install -y procps-ng tar rsync ; microdnf clean all
Copy link
Member

Choose a reason for hiding this comment

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

what's the reason to use ; to join commands?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see a particular reason. This is how it was done in m/s

Copy link
Member

Choose a reason for hiding this comment

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

I like && more but in this case I don't have a strong argument to change

@rbaturov rbaturov force-pushed the must-gather-dockerfile-konflux branch from 21c1341 to 740bd5e Compare January 21, 2025 07:14
@ffromani
Copy link
Member

thanks for the updates! can we move the konflux docker files in (any) konflux-specific directory?

@rbaturov
konflux: add a dockerfile to must gather
8e862d9 
This Dockerfile will be used by konflux.

Signed-off-by: Ronny Baturov <[email protected]>
@rbaturov rbaturov force-pushed the must-gather-dockerfile-konflux branch from 740bd5e to 8e862d9 Compare January 21, 2025 07:29
@ffromani
Copy link
Member

/approve

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Jan 21, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ffromani, rbaturov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 21, 2025
@rbaturov
Copy link
Contributor Author

thanks for the updates! can we move the konflux docker files in (any) konflux-specific directory?

Done

@ffromani
Copy link
Member

/hold cancel

deferring the final lgtm to @shajmakh

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 21, 2025
@shajmakh
Copy link
Member

shajmakh commented Jan 21, 2025 •
edited
Loading

Thanks for the reviews and updates
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 21, 2025
@openshift-merge-bot openshift-merge-bot bot merged commit c17907b into openshift-kni:main Jan 21, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shajmakh shajmakh shajmakh left review comments

@ffromani ffromani ffromani left review comments

@swatisehgal swatisehgal Awaiting requested review from swatisehgal

Assignees

@shajmakh shajmakh

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rbaturov @shajmakh @ffromani