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

When CreatePod fails, dump pod options #2467

Merged
merged 4 commits into from
Nov 14, 2023

Conversation

e-sumin
Copy link
Contributor

@e-sumin e-sumin commented Nov 10, 2023

Change Overview

We have a scenario when invoker can try to create pod with incorrect specification. It would be useful to log what exactly was passed and what pod configuration was built from passed options.

Pull request type

Please check the type of change your PR introduces:

  • 🚧 Work in Progress
  • 🌈 Refactoring (no functional changes, no api changes)
  • 🐹 Trivial/Minor
  • 🐛 Bugfix
  • 🌻 Feature
  • 🗺️ Documentation
  • 🤖 Test

Issues

  • fixes #issue-number

Test Plan

  • 💪 Manual
  • ⚡ Unit test
  • 💚 E2E

@e-sumin e-sumin force-pushed the dump-pod-settings-on-create-pod-failed branch from fb38cbf to 083430a Compare November 14, 2023 09:53
@viveksinghggits
Copy link
Contributor

Can we please add unit test for those newly added utilities.

Kanister automation moved this from In Progress to Reviewer approved Nov 14, 2023
@@ -488,3 +489,59 @@ func GetPodReadyWaitTimeout() time.Duration {

return DefaultPodReadyWaitTimeout
}

// getRedactedEnvVariables returns array of variables with removed values
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe just add to the comment explaining that this is done because the env vars can contain sensitive info

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

@e-sumin
Copy link
Contributor Author

e-sumin commented Nov 14, 2023

Can we please add unit test for those newly added utilities.

Done

@e-sumin e-sumin added the kueue label Nov 14, 2023
@mergify mergify bot merged commit b594d34 into master Nov 14, 2023
14 checks passed
Kanister automation moved this from Reviewer approved to Done Nov 14, 2023
@mergify mergify bot deleted the dump-pod-settings-on-create-pod-failed branch November 14, 2023 16:58
}
return result
}

Copy link
Contributor

Choose a reason for hiding this comment

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

We should also redact or hide the command passed into the pod

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants