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

support command improvements #173

Merged
merged 5 commits into from
Sep 22, 2022
Merged

support command improvements #173

merged 5 commits into from
Sep 22, 2022

Conversation

Itxaka
Copy link
Contributor

@Itxaka Itxaka commented Sep 12, 2022

support: Add k8s logs to support command
support: Rework copyFilesInDir to get full path
support: skip getting k8s resources if no kubeconfig

Signed-off-by: Itxaka igarcia@suse.com

Gathers logs for pods in some namespaces

Signed-off-by: Itxaka <igarcia@suse.com>
@Itxaka Itxaka requested a review from a team September 12, 2022 09:54
Itxaka added 3 commits September 12, 2022 11:57
Reworks copyFilesInDir so it sets the same structure from teh source dir
into the temp dir in case we are storing things in the wrong place.

Also introduces a redacter regex to remove any instances of passwd or
password in the oem files as those are the ones most probable to contain
passwords as part of the cloud-config files of setting the system and we
dont want to store those

Signed-off-by: Itxaka <igarcia@suse.com>
Signed-off-by: Itxaka <igarcia@suse.com>
Signed-off-by: Itxaka <igarcia@suse.com>
@codecov
Copy link

codecov bot commented Sep 12, 2022

Codecov Report

Base: 38.15% // Head: 38.15% // No change to project coverage 👍

Coverage data is based on head (63d724a) compared to base (846c610).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #173   +/-   ##
=======================================
  Coverage   38.15%   38.15%           
=======================================
  Files          10       10           
  Lines         857      857           
=======================================
  Hits          327      327           
  Misses        505      505           
  Partials       25       25           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@davidcassany davidcassany left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@alexander-demicev alexander-demicev left a comment

Choose a reason for hiding this comment

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

looks good overall, 2 minor comments

logrus.Warnf("Failed to get %s on namespace %s: %s", resource, pod, err)
}
// We still want to write the output if the resource was not found or another issue occurred as that can shed info on what's going on
_ = os.WriteFile(fmt.Sprintf("%s/%s-%s-logs.log", dest, resource, pod), out, os.ModePerm)
Copy link
Member

Choose a reason for hiding this comment

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

why did you decide to suppress the error? If that is intentional then suggested change should work too

Suggested change
_ = os.WriteFile(fmt.Sprintf("%s/%s-%s-logs.log", dest, resource, pod), out, os.ModePerm)
os.WriteFile(fmt.Sprintf("%s/%s-%s-logs.log", dest, resource, pod), out, os.ModePerm)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

linting prevents this. Explicitly catch return values

Copy link
Contributor Author

Choose a reason for hiding this comment

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

supressing errors is becuase this si a best-effort-catch-all util. Do not care about errors, not want to show errors to the users, just get as much logs as possible from the system.

func copyDir(srcdir string, destdir string) {
contents, err := os.ReadDir(srcdir)
if err != nil {
return
Copy link
Member

Choose a reason for hiding this comment

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

can add a log entry in case it fails?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as above, dont care if it fails, its not supposed to give errors to the user, just tryu to get as much info as possible otherwise you get an error spouting util on your already broken system. Bad vibes :)

@Itxaka Itxaka merged commit 17d9d21 into rancher:main Sep 22, 2022
@Itxaka Itxaka deleted the extend_support branch September 22, 2022 08:27
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.

3 participants