feat: Add support for plugin UI uploads by disq · Pull Request #18447 · cloudquery/cloudquery

disq · 2024-07-03T16:46:13Z

Part of https://github.com/cloudquery/cloudquery-issues/issues/1864 (internal issue)

hermanschaaf · 2024-07-04T08:26:08Z

+`
+	pluginUIAssetsUploadExample = `
+# Upload plugin UI assets to CloudQuery Hub
+cloudquery plugin uiassets upload test-team/source/test-plugin@v1.0.0`


I think we don't really need a separate command for this--it should be good enough to use cloudquery plugin publish and check for the --ui-dir flag? If the concern is that it's slow to re-upload the binaries every time, maybe let's fix that instead by comparing the checksums before doing a new upload

What if you're not publishing a plugin and simply want to fix a bug in the UI? The command architecture/setup is the same as plugin docs upload (which also offers download and upload --sync though)

What if you're not publishing a plugin and simply want to fix a bug in the UI?

I think it's simpler (and less maintenance) to have one command that does it all -- cloudquery plugin publish no matter what you changed. We just need to make it a bit smarter so it's not a drag to re-upload all the binaries every time.

publish requires a manifest.json so you'd have to create a manifest file every time there's an issue, so you'd have to add a new option to get the plugin name/version directly which would complicate the code and be confusing.

publish requires the plugin to not be published already, so you'd have to add another flag, or maybe make it so that it skips that half of the publush flow when the ui assets flag is specified along with the plugin reference flag

publish is a documented command so you'd have to document all this, with exceptions ("if you use this flag, this portion of the publish flow won't be run", etc)

plugin uiassets is a hidden command with no downside/documentation burden, just like plugin docs is

I see, thanks for the clarification.

I was thinking that the UI publishing would be immutable after the plugin is finalized, so more like binaries, less like docs. If you fix a UI bug, you would upload a new version of the plugin, just like if you fixed a bug in the spec or config validation. I think this is similar to the Grafana model, where a UI bug will remain in a plugin until you upgrade the version.

Not versioning UI changes as part of the plugin version could lead to confusing issues down the line, if these are being cached somewhere other than our own bucket. But okay, I think we can live with it for now if it's a hidden command, meant only to be used in special circumstances.

We're versioning the plugin UI assets separately so that there's no chance of caching issues - each UI "version" has its own uuid prefix. Grafana kept it simple but it's part of their core product, where with us the UI is completely separate from framework/plugins and only used in cloud. Because of this separate versioning needed to happen to keep things sane. Not to mention each plugin version/release is customer-facing and releasing multiple versions with no visible/CLI-affecting changes might potentially cause grief with other teams or the users themselves.

What will the workflow be if we'd need to support some UI issue for a plugin? I've seen in support that we end up having to try a sync by checking out some tag of an older version, because the customer hasn't/can't update to the latest version.

If we had a separate UUID-based versioning scheme for the UI Framework, would it be cumbersome to see a certain version of the UI?

I had a chat with @hermanschaaf about this to get context and I tend to agree with having separate commands to begin with until we have some real-world experience and see the tradeoffs better.

Co-authored-by: Herman Schaaf <hermanschaaf@gmail.com>

marianogappa

Added comments.

marianogappa · 2024-07-04T09:49:06Z

+			w.Write([]byte(`{}`))
+		case "/plugins/cloudquery/source/test/versions/v1.2.3/uiassets":
+			checkAuthHeader(t, r)
+			if r.Method == http.MethodPost {


Two ifs ending in return and a default case at the end 🤔 This is a camouflaged switch statement.

Correct but linter would've shouted at me for having that probably 😂

Again, I wouldn't write this like that if it wasn't a one time test code.

OK added double switch in 51bf299 😂

marianogappa · 2024-07-04T10:11:05Z

+`
+	pluginUIAssetsUploadExample = `
+# Upload plugin UI assets to CloudQuery Hub
+cloudquery plugin uiassets upload test-team/source/test-plugin@v1.0.0`


What will the workflow be if we'd need to support some UI issue for a plugin? I've seen in support that we end up having to try a sync by checking out some tag of an older version, because the customer hasn't/can't update to the latest version.

If we had a separate UUID-based versioning scheme for the UI Framework, would it be cumbersome to see a certain version of the UI?

marianogappa · 2024-07-04T10:25:03Z

+		return "", fmt.Errorf("failed to open file: %w", err)
+	}
+	defer fp.Close()
+	if _, err := fp.Read(filebytes); err != nil {


I think this fails if the file is < 512 bytes.

if err != nil && err != os.EOF { return "", fmt.Errorf("failed to read file: %w", err) }

The test passes with index.html which is a strong boo, so it seems to work... rest of the filebytes would be left as zero bytes, probably.

marianogappa · 2024-07-04T10:40:49Z

+`
+	pluginUIAssetsUploadExample = `
+# Upload plugin UI assets to CloudQuery Hub
+cloudquery plugin uiassets upload test-team/source/test-plugin@v1.0.0`


I had a chat with @hermanschaaf about this to get context and I tend to agree with having separate commands to begin with until we have some real-world experience and see the tradeoffs better.

erezrokah · 2024-07-08T08:41:47Z

 		Set("destination_paths", destinationPaths).
 		Set("user_id", details.user.ID).
-		Set("user_email", string(details.user.Email))
+		Set("user_email", details.user.Email)


What's the reason for this change? This was a bug fix #18208

🤖 I have created a release *beep* *boop* --- ## [5.25.0](cli-v5.24.0...cli-v5.25.0) (2024-07-09) ### Features * Add support for plugin UI uploads ([#18447](#18447)) ([54b829e](54b829e)) * Unhide `--tables-metrics-location` flag, add duration to table ([#18498](#18498)) ([83948b5](83948b5)) ### Bug Fixes * Attempt mitigation of race in tests. ([#18446](#18446)) ([f9774c1](f9774c1)) * **deps:** Update golang.org/x/exp digest to 7f521ea ([#18428](#18428)) ([5d18290](5d18290)) * **deps:** Update module github.com/cloudquery/cloudquery-api-go to v1.12.0 ([#18448](#18448)) ([a5850e1](a5850e1)) * **deps:** Update module github.com/cloudquery/cloudquery-api-go to v1.12.1 ([#18478](#18478)) ([f58c2b7](f58c2b7)) * **deps:** Update module github.com/cloudquery/codegen to v0.3.17 ([#18491](#18491)) ([b43fd16](b43fd16)) * **deps:** Update module github.com/cloudquery/plugin-pb-go to v1.20.3 ([#18495](#18495)) ([86fac37](86fac37)) * **deps:** Update module github.com/cloudquery/plugin-sdk/v4 to v4.49.1 ([#18497](#18497)) ([3416eb7](3416eb7)) * **deps:** Update module github.com/docker/docker to v26.1.4+incompatible ([#18435](#18435)) ([4bbaece](4bbaece)) * **deps:** Update module github.com/getsentry/sentry-go to v0.28.1 ([#18436](#18436)) ([bb88a05](bb88a05)) * **deps:** Update module github.com/rs/cors to v1.11.0 [SECURITY] ([#18482](#18482)) ([2ab40a3](2ab40a3)) * **deps:** Update module github.com/schollz/progressbar/v3 to v3.14.4 ([#18437](#18437)) ([9c089f5](9c089f5)) * **deps:** Update module google.golang.org/grpc to v1.65.0 ([#18488](#18488)) ([4d6343d](4d6343d)) * Don't close metrics file before last print ([#18499](#18499)) ([efb8119](efb8119)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

feat: Add support for plugin UI uploads

fe2253f

disq requested review from a team and marianogappa and removed request for a team July 3, 2024 16:46

cq-bot added area/cli area/website labels Jul 3, 2024

vercel Bot deployed to Preview July 3, 2024 16:47 View deployment

disq added 2 commits July 3, 2024 17:51

Unrelated lint

245752b

Add test

e604a2a

disq requested a review from hermanschaaf July 3, 2024 17:14

disq added 7 commits July 3, 2024 18:14

Add testdata

57d26f9

fix test

b80472d

Support "./" syntax

ce5c1e1

cleaner content-type

6c946ce

upload in parallel

9d9e55c

always require an index.html file

6fb3140

add mutex to test

756fa83

hermanschaaf reviewed Jul 4, 2024

View reviewed changes

Comment thread cli/cmd/testdata/ui-build/index.html Outdated

hermanschaaf reviewed Jul 4, 2024

View reviewed changes

Comment thread cli/internal/publish/images/images.go Outdated

disq and others added 2 commits July 4, 2024 09:38

Update cli/cmd/testdata/ui-build/index.html: Use a strong boo

b20e4fc

Co-authored-by: Herman Schaaf <hermanschaaf@gmail.com>

Add comment

b9f15af

hermanschaaf approved these changes Jul 4, 2024

View reviewed changes

Merge branch 'main' into feat/add-support-for-plugin-ui-uploads

9ffcd2d

marianogappa approved these changes Jul 4, 2024

View reviewed changes

Add switch inside switch

51bf299

disq added the automerge Automatically merge once required checks pass label Jul 4, 2024

kodiakhq Bot merged commit 54b829e into main Jul 4, 2024

kodiakhq Bot deleted the feat/add-support-for-plugin-ui-uploads branch July 4, 2024 11:06

cq-bot mentioned this pull request Jul 4, 2024

chore(main): Release cli v5.25.0 #18430

Merged

erezrokah reviewed Jul 8, 2024

View reviewed changes

Conversation

disq commented Jul 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

disq Jul 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

marianogappa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

disq commented Jul 3, 2024 •

edited

Loading

disq Jul 4, 2024 •

edited

Loading