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

instead hard code by variable in error message #735

Merged
merged 2 commits into from
Jun 26, 2018
Merged

instead hard code by variable in error message #735

merged 2 commits into from
Jun 26, 2018

Conversation

xiekeyang
Copy link
Contributor

@xiekeyang xiekeyang commented Jun 8, 2018

ArchiveLiteralSizeLimit has constant value. When encountering the package error here, we should warn the exact ArchiveLiteralSizeLimit value, but not 256KB hard code.


This change is Reviewable

@@ -64,12 +66,14 @@ func (a *API) PackageApiCreate(w http.ResponseWriter, r *http.Request) {

// Ensure size limits
if len(f.Spec.Source.Literal) > int(fission.ArchiveLiteralSizeLimit) {
err := fission.MakeError(fission.ErrorInvalidArgument, "Package literal larger than 256K")
err := fission.MakeError(fission.ErrorInvalidArgument,
fmt.Sprintf("Package literal larger than 0x%s", strconv.FormatInt(fission.ArchiveLiteralSizeLimit, 16)))
Copy link
Member

Choose a reason for hiding this comment

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

The error is for humans, plain old decimal is probably better. Also, please include units (bytes/kb/etc).

@xiekeyang
Copy link
Contributor Author

@soamvasani I introduced the package github.com/dustin/go-humanize and formatted the message value to human readable size. Re-Committed, PTAL, thanks a lot!

@soamvasani
Copy link
Member

LGTM.

@soamvasani soamvasani added this to the 0.9.0 milestone Jun 25, 2018
This introduces human size package, to indicate number by human readable string.
…izeLimit value

This prints error message to indicate `ArchiveLiteralSizeLimit` value.
@smruthi2187 smruthi2187 merged commit 09b62e5 into fission:master Jun 26, 2018
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