feat: Upload images from markdown files to CDN when publishing#16033
Conversation
hermanschaaf
left a comment
There was a problem hiding this comment.
Did a first pass review; I think overall it looks good enough for a first version. Most of my comments are just suggestions for possible improvement, perhaps worth some comments in the code at least if we don't want to improve them now.
My two main comments are:
- the one about the
replacedirective ongithub.com/cloudquery/cloudquery-api-gobefore merging - the one about the test cases seeming hard to write / validate, I think it would make sense to split the tests
| } | ||
| p := goldmark.New( | ||
| goldmark.WithParserOptions( | ||
| parser.WithASTTransformers(util.Prioritized(imf, 999999)), |
There was a problem hiding this comment.
I'm not sure if this priority matters, or if it should be lowest
candiduslynx
left a comment
There was a problem hiding this comment.
just some suggestions
| checksum, name := refParts[0], refParts[1] | ||
|
|
||
| reqs = append(reqs, cloudquery_api.TeamImageCreate{ | ||
| Name: name, |
There was a problem hiding this comment.
It might be solved elsewhere, but we shouldn't overwrite files in storage (i.e., every new uploaded version should be treated as a separate file, rather than the same one).
Although it might be OK to just use name + checksum as a key, there might be collisions still, & the data overwrite can result in some odd artifacts in the old versions of the doc page.
We also should think about file size limits (it's funny how many people would use anything as free file storage given the opportunity).
There was a problem hiding this comment.
You're correct it's solved elsewhere (see the relevant PR in the backend repo) the backend will do a HEAD and if there is an object on the same 'calculated path' it won't return an upload url.
There was a problem hiding this comment.
File size limits are also not a discussion for clients, we also have the ability to lock out each team from uploading.
| for _, p := range posList { | ||
| if p.end { | ||
| open-- | ||
| } else { | ||
| open++ | ||
| } | ||
| if open > 1 { | ||
| return nil, fmt.Errorf("found overlapping range: %d-%d", lastPos, p.at) | ||
| } | ||
| lastPos = p.at | ||
| } |
There was a problem hiding this comment.
| for _, p := range posList { | |
| if p.end { | |
| open-- | |
| } else { | |
| open++ | |
| } | |
| if open > 1 { | |
| return nil, fmt.Errorf("found overlapping range: %d-%d", lastPos, p.at) | |
| } | |
| lastPos = p.at | |
| } | |
| for i, r := range posList[1:] { | |
| l := posList[i] | |
| if l.end >= r.start { | |
| return nil, fmt.Errorf("found overlapping range: %d-%d", r.start, l.end) | |
| } | |
| } |
There was a problem hiding this comment.
how about positions starting at the same point? this start/end stuff leads to more complicated code for this use case.
There was a problem hiding this comment.
Also see @hermanschaaf 's comment in #16033 (comment)
🤖 I have created a release *beep* *boop* --- ## [5.0.0](cli-v4.4.0...cli-v5.0.0) (2024-01-12) ### ⚠ BREAKING CHANGES * Remove deprecated top level spec options (#15999). The following deprecated spec options were removed: * source plugin top level spec: * `concurrency` * `table_concurrency` * `resource_concurrency` * `backend` * `backend_spec` * destination plugin top level spec: * `batch_size` * `batch_size_bytes` **For most users this shouldn't be a breaking change**. We've deprecated these options a while ago and some were moved to the plugin level spec. If you were using the deprecated options on a CLI version lower than `v5.0.0` you should have gotten a warning about it. ### Features * Add JSON schema for CLI specs ([#15998](#15998)) ([da02049](da02049)) * Remove deprecated spec options (#15999) ([9e25f4a](9e25f4a)) * Upload images from markdown files to CDN when publishing ([#16033](#16033)) ([cf7cdb1](cf7cdb1)) ### Bug Fixes * Add `X-Meta-User-Team-Name` during docker push ([#16013](#16013)) ([129b7c2](129b7c2)) * Add missing `X-Meta-User-Team-Name` header and manifest types option ([#16113](#16113)) ([9d8899e](9d8899e)) * **deps:** Update github.com/apache/arrow/go/v15 digest to 6d44906 ([#16115](#16115)) ([8b0ae62](8b0ae62)) * **deps:** Update module github.com/cloudquery/cloudquery-api-go to v1.6.4 ([#16067](#16067)) ([2e7b7d6](2e7b7d6)) * **deps:** Update module github.com/cloudquery/plugin-pb-go to v1.16.2 ([#15948](#15948)) ([2def2ef](2def2ef)) * **deps:** Update module github.com/cloudquery/plugin-pb-go to v1.16.3 ([#16002](#16002)) ([e2d5605](e2d5605)) * **deps:** Update module github.com/cloudquery/plugin-pb-go to v1.16.4 ([#16126](#16126)) ([6a776ae](6a776ae)) * **deps:** Update module github.com/cloudquery/plugin-sdk/v4 to v4.25.0 ([#15932](#15932)) ([2292b5a](2292b5a)) * **deps:** Update module github.com/cloudquery/plugin-sdk/v4 to v4.25.1 ([#16069](#16069)) ([edda65c](edda65c)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Limited testing results:
Newly supported:
HTML<img>tags in markdown (will try to fix this, but need to make sure it doesn't replace inside code blocks)Reference-style image syntax (with ids, documented here)