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

Update mustgather.go #1745

Open
wants to merge 1 commit into
base: release-4.14
Choose a base branch
from

Conversation

danielyeap
Copy link

(1) ability to send local files into the mustgather pods (needed for custom mustgather tool)
(2) ability to run the mustgather pod using a user-defined user (instead of fixed to the "default" user/serviceaccount)

(1) ability to send local files into the mustgather pods (needed for custom mustgather tool)
(2) ability to run the mustgather pod using a user-defined user (instead of fixed to the "default" user/serviceaccount)
Copy link
Contributor

openshift-ci bot commented Apr 25, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: danielyeap
Once this PR has been reviewed and has the lgtm label, please assign soltysh for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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 needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 25, 2024
Copy link
Contributor

openshift-ci bot commented Apr 25, 2024

Hi @danielyeap. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@danielyeap
Copy link
Author

Hi @ardaguclu @deads2k
Anything pending on my side for this review?
Sorry...this is my first time.
Thanks.

@ardaguclu
Copy link
Member

@danielyeap thank you for spending time on this PR. However, I don't think we can merge the changes in here.
The reasons are:

  • Adding a new flag would require strong justification and even the ones that are marked as hidden is there for a reason. Especially, there are multiple images exist for must-gather, we can't add new flags without notifying the all image owners.
  • We have to be backwards compatible. All the changes are only be applied if they are working against older images, OCP version, k8s version etc. I saw a couple of changes are not backwards compatible.

I hope it makes sense. Thanks again.

@danielyeap
Copy link
Author

Hi @ardaguclu
Thanks for spending time to respond.

I hope you can help with a few questions:
(1) Can you point me to all the must-gather images?
(2) What are the changes that are not backward-compatible?

Lastly, while there are 2 features added through this code change, the truly critical one is the ability to send files into the mustgather pod.

Any idea whether that is a planned feature for the future please?

Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants