Skip to content

Fix empty gather logs due to script crash#33

Merged
jannfis merged 8 commits into
redhat-developer:mainfrom
olivergondza:fix-eventually-empty-process-substitutions
Sep 4, 2025
Merged

Fix empty gather logs due to script crash#33
jannfis merged 8 commits into
redhat-developer:mainfrom
olivergondza:fix-eventually-empty-process-substitutions

Conversation

@olivergondza

@olivergondza olivergondza commented Sep 1, 2025

Copy link
Copy Markdown
Collaborator

This is a regression form 1.17[1], works in 1.16. Script fails with error: resource name may not be empty. This happens when the added quoting in oc get ... "$(xxx)" ... caused the argument is now empty instead of missing.

[1] https://github.com/redhat-developer/gitops-must-gather/pull/31/files

What type of PR is this?

Uncomment only one /kind line, and delete the rest.
For example, > /kind bug would simply become: /kind bug

/kind bug

/kind cleanup
/kind failing-test
/kind enhancement
/kind documentation
/kind code-refactoring

What does this PR do / why we need it:

Fixes the bug. Improved error reporting. Refactors the code so shellcheck can be happy. This also enabled removing following in the future:

# Getting non.existent.crd is a hack to avoid getting all available crds in the cluster in case there are no owned resources that do not contain "argoproj.io"

Have you updated the necessary documentation?

  • [n/a] Documentation update is required by this PR.
  • [n/a] Documentation has been updated.

Which issue(s) this PR fixes:

None

Test acceptance criteria:

  • Unit Test
  • E2E Test

How to test changes / Special notes to the reviewer:

This was verified by running locally and compared to the results of oc adm must-gather --image=registry.redhat.io/openshift-gitops-1/must-gather-rhel8:v1.16.0.

@olivergondza
fix(gather_gitops.sh): Unquote process substitutions delivering lists
f8b55e4 
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
@olivergondza
fix(gather_gitops.sh): Replace process substitutions delivering lists…
976cbbe 
… with readarray

Signed-off-by: Oliver Gondža <ogondza@gmail.com>
@openshift-ci

openshift-ci Bot commented Sep 1, 2025

Copy link
Copy Markdown

@olivergondza: The label(s) kind/bug cannot be applied, because the repository doesn't have them.

Details

In response to this:

This is a regression form 1.17[1], works in 1.16. Script fails with error: resource name may not be empty. This happens when the added quoting in oc get ... "$(xxx)" ... caused the argument is now empty instead of missing.

[1] https://github.com/redhat-developer/gitops-must-gather/pull/31/files

What type of PR is this?

Uncomment only one /kind line, and delete the rest.
For example, > /kind bug would simply become: /kind bug

/kind bug

/kind cleanup
/kind failing-test
/kind enhancement
/kind documentation
/kind code-refactoring

What does this PR do / why we need it:

Fixes the bug. Improved error reporting. Refactors the code so shellcheck can be happy. This also enabled removing following in the future:

# Getting non.existent.crd is a hack to avoid getting all available crds in the cluster in case there are no owned resources that do not contain "argoproj.io"

Have you updated the necessary documentation?

  • [n/a] Documentation update is required by this PR.
  • [n/a] Documentation has been updated.

Which issue(s) this PR fixes:

None

Test acceptance criteria:

  • Unit Test
  • E2E Test

How to test changes / Special notes to the reviewer:

This was verified by running locally and compared to the results of oc adm must-gather --image=registry.redhat.io/openshift-gitops-1/must-gather-rhel8:v1.16.0.

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-sigs/prow repository.

@olivergondza
fix(gather_gitops.sh): Silent shellcheck false positive
d617636 
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
@olivergondza

olivergondza commented Sep 2, 2025

Copy link
Copy Markdown
Collaborator Author

Working on contributing a verification for this. For now, cluster-scoped-resources/apiextensions.k8s.io/customresourcedefinitions/gitopsservices.pipelines.openshift.io.yaml seems to be missing for reasons yet to be determined.

@olivergondza
fix(gather_gitops.sh): Fix receiving gitopsservices.pipelines.openshi…
25ff78f 
…ft.io

Signed-off-by: Oliver Gondža <ogondza@gmail.com>
@olivergondza

olivergondza commented Sep 2, 2025 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Alright, all fixed. The output can be compared with 1.16 using:

./test/compare.sh registry.redhat.io/openshift-gitops-1/must-gather-rhel8:v1.16.0 quay.io/ogondza/gitops-must-gather:490ce444c73266f5a008425fd5464ac1436e967b

Edit: Tag updated

@olivergondza
feat(test/compare.sh): Add verification
d8e1000 
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
olivergondza
olivergondza commented Sep 2, 2025
View reviewed changes
Comment thread README.md

```shell
chmod +x ./gather_gitops.sh
./gather_gitops.sh --base-collection-path .

@olivergondza olivergondza Sep 2, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Does not work since #23.

@olivergondza
feat(test/compare.sh): Shellcheck
272f170 
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
@olivergondza
feat(gather_gitops.sh): Do not issue errors when gitops not installed
490ce44 
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
@jannfis

jannfis commented Sep 4, 2025

Copy link
Copy Markdown
Member

/ok-to-test

jannfis
jannfis reviewed Sep 4, 2025
View reviewed changes
Comment thread gather_gitops.sh Outdated
anandf
anandf reviewed Sep 4, 2025
View reviewed changes

@anandf anandf left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@olivergondza
fix(gather_gitops.sh): Handle no csv,sub,ip explicitly
85ba013 
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
@jannfis

jannfis commented Sep 4, 2025

Copy link
Copy Markdown
Member

/ok-to-test

@jannfis

jannfis commented Sep 4, 2025

Copy link
Copy Markdown
Member

/lgtm

@openshift-ci openshift-ci Bot added the lgtm label Sep 4, 2025
@jannfis

jannfis commented Sep 4, 2025

Copy link
Copy Markdown
Member

/approve

1 similar comment
@jannfis

jannfis commented Sep 4, 2025

Copy link
Copy Markdown
Member

/approve

jannfis
jannfis approved these changes Sep 4, 2025
View reviewed changes

@jannfis jannfis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@jannfis jannfis merged commit 135850b into redhat-developer:main Sep 4, 2025
4 of 5 checks passed
@olivergondza olivergondza deleted the fix-eventually-empty-process-substitutions branch September 4, 2025 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jannfis jannfis jannfis approved these changes
@jaideepr97 jaideepr97 Awaiting requested review from jaideepr97
@reginaelyse reginaelyse Awaiting requested review from reginaelyse
+1 more reviewer
@anandf anandf anandf left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@jannfis jannfis

Labels

lgtm ok-to-test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@olivergondza @jannfis @anandf