Improve the error description when no name is provided in func delete

[RayyanSeliya]

Changes

• 🧹 Fix confusing UX in func delete when no function name provided
• 🧹 Replace cryptic "name required" error with clear guidance
• 🧹 Show educational error message with deletion workflow examples and help pointer

/kind enhancement

Fixes #3053

Previously, func delete would show a cryptic "name required" error that didn't help users understand how to delete functions. Now it immediately shows clear guidance on the two ways to delete functions (by name or by path) with practical examples.
Follows two layer approach as suggested to do so !

Release Note

Improve func delete user experience by replacing cryptic "name required" error with clear guidance on how to delete functions using name or path options.

[knative-prow]

Hi @RayyanSeliya. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[codecov]

Codecov Report

❌ Patch coverage is 13.51351% with 32 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.06%. Comparing base (bfd65ae) to head (9c9fc4a).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
cmd/delete.go	13.51%	29 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3054      +/-   ##
==========================================
+ Coverage   55.66%   59.06%   +3.39%     
==========================================
  Files         134      134              
  Lines       17315    17350      +35     
==========================================
+ Hits         9638    10247     +609     
+ Misses       6773     6136     -637     
- Partials      904      967      +63     

Flag	Coverage Δ
e2e-tests	40.26% <13.51%> (+10.51%)	⬆️
integration-tests	53.82% <13.51%> (+1.57%)	⬆️
unit-tests	46.19% <13.51%> (-0.08%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[lkingland]

This is good! I would just use a general fn.ErrNameRequired as the error's name. The fact it's "for delete" is implied by its context here (so you can reuse ErrNameRequired in other places where... a name is required)

[lkingland]

This is exactly what I was thinking as well: Print the error message as it was returned from the client, and then append "cli specific" text to help out more.

Since this text is a bit long, you might want to consider breaking this out into either a separate function dedicated to error formatting, or create an "cli specific" version of the error with the full text, and the layer 1 error as a member:

return ErrDeleteNameRequired(err)

where err is a fn.ErrNameRequired and ErrDeleteNameRequired returns the CLI-specific error you have here.

Experiment with different patterns/structures to see what flows well.

Good start

[RayyanSeliya]

hey @lkingland @gauron99 i just wanted to tell that in this case as i have created an explicit function for the typed error coz its a bit lengthy so i think in future we will need to shift this type of implementation coz it will affect the code readability as david sir you mentioned like we can create an explicit file in cmd/errors.go or something like that as if i will go forward for implementing the issues i have mentioned in the docs of func build and func deploy it will get flooded by the long description in the code it will be so messy what are your thoughts on this ???

[lkingland]

Code Location:

I think keeping this error in the command's file (delete.go) at the bottom of the file is fine because it is specific to a given command. If you end up creating shared errors for the CLI, then a cmd/errors.go file would be a good place for those. The core errors (like fn.ErrNameRequired) are shared, so putting them in a dedicated file (pkg/functions/errors.go) is appropriate.

If you end up with more than a thousand lines of error code (approximately), it might be worth creating a dedicated file for them, cmd/delete_errors.go

Basic Implementation Suggestion:

Here's a suggestion on how to use a custom error types to help make this a bit cleaner:

// ErrDeleteNameRequired wraps core library errors with CLI-specific context
// for delete operations that require a function name or path.
type ErrDeleteNameRequired struct {
	// Underlying error from the core library (e.g., fn.ErrNameRequired)
	Err error
}

// NewErrDeleteNameRequired creates a new ErrDeleteNameRequired wrapping the given error
func NewErrDeleteNameRequired(err error) ErrDeleteNameRequired {
	return ErrDeleteNameRequired{Err: err}
}

// Error implements the error interface with CLI-specific help text
func (e ErrDeleteNameRequired) Error() string {
	return fmt.Sprintf(`%v
Function name is required for deletion (or --path not specified).
You can delete functions in two ways:
1. By name:
   func delete myfunction                     Delete function by name
   func delete myfunction --namespace apps    Delete from specific namespace
2. By path:
   func delete --path /path/to/function       Delete function at specific path
Examples:
   func delete myfunction                     Delete 'myfunction' from cluster
   func delete myfunction --namespace prod    Delete from 'prod' namespace
   func delete --path ./myfunction            Delete function at path
For more options, run 'func delete --help'`, e.Err)
}

The naming convention here would be "Err{Command Name}{Core Error Name}"

Complex Errors Implementation Suggestion

Now also consider how this can help with the creation of more complex help text strings by allowing you to add more context to the type as needed:

// NewErrDeleteNamespaceNotFound creates a new ErrDeleteNamespaceNotFound with the given error and namespace
func NewErrDeleteNamespaceNotFound(err error, namespace string) *ErrDeleteNamespaceNotFound {
	return &ErrDeleteNamespaceNotFound{
		Err:       err,
		Namespace: namespace,
	}
}

// Error implements the error interface with CLI-specific help text
func (e *ErrDeleteNamespaceNotFound) Error() string {
	return fmt.Sprintf(`%v
Namespace '%s' does not exist or is not accessible.

To resolve this issue:

{in-depth help text here}

For more options, run 'func delete --help'`, 
		e.Err, 
		e.Namespace)
}

Future Flexibility and Justification

This may seem like it's no different than the formatting function you suggested already, but consider how it allows us to do interesting things like this in the future if we need to:

// Unwrap returns the underlying error for use with errors.Is/As
func (e *ErrDeleteNameRequired) Unwrap() error {
	return e.Err
}

// Is allows this error to match the underlying error type
// This enables errors.Is(err, fn.ErrNameRequired) to work through the wrapper
func (e *ErrDeleteNameRequired) Is(target error) bool {
	return errors.Is(e.Err, target)
}

(No need to implement those methods now, however; that's just to give you an example of the flexibility afforded when using types.

[RayyanSeliya]

thanks @lkingland for the clarification i will follow your suggestion and keep the error in the command file delete.go as of now if the code grows too much coz of typed errors we can move it into a separate file like cmd/delete_errors.go . And for shared/common cases, we’ll keep them in the dedicated locations as pkg/functions/errors.go

[gauron99]

/retest

[gauron99]

/retest

[lkingland]

/retest

[lkingland]

/lgtm

Just need those tests to pass..

[gauron99]

It looks like you might need to rebase @RayyanSeliya . This looks like its missing https://github.com/knative/func/pull/3055/files

TLDR: theres been changes in one of the dependencies we use to create clusters for these tests and the instalation changed, hence none of the tests that use a cluster will work

[RayyanSeliya]

Sure @gauron99 will update the branch I have seen many chats in these days on slack 🙂 going on so will do it after my exam today in theate evening I was busy with my exams so was inactive !! Thx !

[RayyanSeliya]

hey @gauron99 is this quarkus test related to the changes in the pr i guess no i have updated the branch can have a look now

[gauron99]

/lgtm

[knative-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gauron99, lkingland, RayyanSeliya

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [gauron99,lkingland]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment