-
-
Notifications
You must be signed in to change notification settings - Fork 5
Closes #20 :Add Tags to every uploaded asset #21
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
base: main
Are you sure you want to change the base?
Conversation
README.md
Outdated
| - `-checksums_file`: Path to the checksums file (default: `checksums.csv`) | ||
| - `-download_jpg_from_jxl`: Converts JXL images to JPG on download for compatibility (default: `false`) | ||
| - `-download_jpg_from_avif`: Converts AVIF images to JPG on download for compatibility (default: `false`) | ||
| - `-tag-ids`: Comma-separated list of tag IDs to add to every asset (default: `""`) |
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.
wrong
| req.Header.Set("Content-Type", multipartWriter.FormDataContentType()) | ||
| // Send the request to the upstream server | ||
| resp, err := getHTTPclient().Do(req) | ||
| defer resp.Body.Close() |
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.
why move this defer? can return without closing body on error
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.
Because I thought it better to check for errors before deferring.
| return fmt.Errorf("no asset ID found in upstream response") | ||
| } | ||
| // Tag the asset | ||
| if len(tagIDs) > 0 { |
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.
should be checked before
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.
what do you suggest? Either way a check has to be put here to see whether this request is needed or not since it should be optional for users to add tags or not.
jobs.go
Outdated
|
|
||
| // Create tag request | ||
| tagURL := fmt.Sprintf("%s/api/tags/assets", upstreamURL) | ||
| tagReq, err := http.NewRequestWithContext(ctx, "PUT", tagURL, bytes.NewReader(tagBodyBytes)) |
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.
why with context?
main.go
Outdated
|
|
||
| // goreleaser auto updated vars | ||
| var version = "dev" | ||
| var version = "v0.8.1" |
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.
must not modify
main.go
Outdated
| } else { | ||
| log.Printf("no tmp directory set, uploaded files will be saved on disk multiple times, this can shorten your disk lifespan !") | ||
| } | ||
| // Process tags |
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.
move to init
| } | ||
|
|
||
| // getTagIDs parses tagIDsStr into a list of tag IDs | ||
| func getTagIDs() ([]string, error) { |
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.
I would just enforce a strict formatting, user error if they put spaces
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.
done, kept a check to make it easier to debug.
main.go
Outdated
| flag.StringVar(&checksumsFile, "checksums_file", viper.GetString("checksums_file"), "Path to the checksums file") | ||
| flag.BoolVar(&downloadJpgFromJxl, "download_jpg_from_jxl", viper.GetBool("download_jpg_from_jxl"), "Converts JXL images to JPG on download for wider compatibility") | ||
| flag.BoolVar(&downloadJpgFromAvif, "download_jpg_from_avif", viper.GetBool("download_jpg_from_avif"), "Converts AVIF images to JPG on download for wider compatibility") | ||
| flag.StringVar(&tagIDsStr, "tag-ids", viper.GetString("tag_ids"), "Comma-separated list of tag IDs") |
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.
mixing - and _
jobs.go
Outdated
| tagReq.AddCookie(cookie) | ||
| } | ||
| // Copy other headers that might be relevant for authentication | ||
| if apiKey := originalReq.Header.Get("x-api-key"); apiKey != "" { |
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.
there is helper functions to copy all headers/cookies
The goal of this PR is to let the user add a list of tags to every uploaded assets.
This is quite useful since it is a pain to set up a separate logic to bulk tag assets for multiple reasons:
/search/metadatacontains assetIDs but not tags, not even whether it has tags or not likehasMetadata.Let me know what you think about this.
If needed I can add the possibility to use tag names instead of IDs and make sure they exist before tagging assets.