-
Notifications
You must be signed in to change notification settings - Fork 674
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
chore: handle empty or invalid secrets nicely #4801
Conversation
Thank you for opening this pull request! 🙌 These tips will help get your PR across the finish line:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4801 +/- ##
==========================================
+ Coverage 61.08% 61.10% +0.02%
==========================================
Files 794 794
Lines 51203 51213 +10
==========================================
+ Hits 31277 31295 +18
+ Misses 17043 17037 -6
+ Partials 2883 2881 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@trutx some tests are failing. mind taking a look
Cc @katrogan |
Hey @pingsutw is there anything you need to get this PR merged? |
@trutx sorry I missed it. Could you merge the master into your branch? |
I did, but tests seem to be stuck? Maybe you need to reapprove @pingsutw ? |
Wow this should be small but has a lot of files impacted? |
Used the github "sync fork" button to merge from upstream/master. I guess a rebase was a better option. 1 sec. |
Signed-off-by: Roger Torrentsgenerós <[email protected]>
Signed-off-by: Roger Torrentsgenerós <[email protected]>
Signed-off-by: Roger Torrentsgenerós <[email protected]>
Signed-off-by: Roger Torrentsgenerós <[email protected]>
Signed-off-by: Roger Torrentsgenerós <[email protected]>
This CI check failure is going to be fixed by flyteorg/flytesnacks#1670. |
Congrats on merging your first pull request! 🎉 |
* chore: handle empty or invalid secrets nicely Signed-off-by: Roger Torrentsgenerós <[email protected]> * chore: this belongs inside the if clause Signed-off-by: Roger Torrentsgenerós <[email protected]> * test: use old key too Signed-off-by: Roger Torrentsgenerós <[email protected]> * test: cover newly added code Signed-off-by: Roger Torrentsgenerós <[email protected]> * chore: make linter happy Signed-off-by: Roger Torrentsgenerós <[email protected]> --------- Signed-off-by: Roger Torrentsgenerós <[email protected]>
* chore: handle empty or invalid secrets nicely Signed-off-by: Roger Torrentsgenerós <[email protected]> * chore: this belongs inside the if clause Signed-off-by: Roger Torrentsgenerós <[email protected]> * test: use old key too Signed-off-by: Roger Torrentsgenerós <[email protected]> * test: cover newly added code Signed-off-by: Roger Torrentsgenerós <[email protected]> * chore: make linter happy Signed-off-by: Roger Torrentsgenerós <[email protected]> --------- Signed-off-by: Roger Torrentsgenerós <[email protected]>
Tracking issue
Closes #4800
Why are the changes needed?
So flyteadmin errors out nicely and with information about the error instead of with a stack trace, which is difficult to follow for people not used to code.
What changes were proposed in this pull request?
I'm adding some missing error handling.
How was this patch tested?
Having both an empty and an invalid
PrivateKeyPEM
variable resulted in the stack trace below:This refers to these fields in the
flyteadmin_config.yaml
file:.auth.appAuth.selfAuthServer.tokenSigningRSAKeySecretName
.clusters.clusterConfigs[].auth.certPath
Setting valid PEMs in both cases solved the issue. Values were read from files mounted in the pod from a Kubernetes
Secret
, so the actual fix was to correct the value in theSecret
.Setup process
Set an empty environment variable or an invalid value in the
Secret
.Check all the applicable boxes