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

fix: avoid panic when not passing AuthSupplier #9586

Merged
merged 1 commit into from
Sep 19, 2022

Conversation

wujunwei
Copy link
Contributor

@wujunwei wujunwei commented Sep 14, 2022

Signed-off-by: wujunwei [email protected]

Fixes #9585

Fix panic: runtime error: invalid memory address or nil pointer dereference

@wujunwei
Copy link
Contributor Author

/retest

@wujunwei wujunwei force-pushed the fix-auth-supplier-panic branch 5 times, most recently from 4086e8f to 7d29202 Compare September 15, 2022 01:27
if opts.ArgoServerOpts.HTTP1 {
return newHTTP1Client(opts.ArgoServerOpts.GetURL(), opts.AuthSupplier(), opts.ArgoServerOpts.InsecureSkipVerify, opts.ArgoServerOpts.Headers)
if opts.AuthSupplier != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Should we check auth supplier and return error instead? Otherwise it's auth=""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we check auth supplier and return error instead? Otherwise it's auth=""

Of course, but we can only do something like below when there is no any Authentication.

apiclient.Opts{
			ArgoServerOpts: ArgoServerOpts,
			AuthSupplier: func() string {
				return ""
			},
		})

@terrytangyuan terrytangyuan enabled auto-merge (squash) September 19, 2022 21:07
@terrytangyuan terrytangyuan merged commit 72d3599 into argoproj:master Sep 19, 2022
juchaosong pushed a commit to juchaosong/argo-workflows that referenced this pull request Nov 3, 2022
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.

panic when not passing AuthSupplier option
3 participants