Get the `create-report` async task in shape #138

bcspragu · 2024-01-12T22:38:48Z

This PR makes assorted tweaks to the runner's create-report task, like:

Adding new pacta.FileTypes for assorted HTML-adjacent types present in the report (see below)
- Includes an UNKNOWN type in case I miss anything, we can always go back and re-label them based on file extension
Fix some issues with paths, use the portfolio UUID as the directory prefix (to avoid collisions)
Publish the event when we're done so the main app can record the info

Mostly unrelated changes:

Fixes Vue warnings about setup-only functionality called outside setup #135 by pulling handleMissingTranslation from the $nuxt object instead of inject
Add a helper script for iterating quickly on the runner, build_and_load_runner.sh

Some resources that get created from this:

select * from blob WHERE blob_uri LIKE 'az://reports/%';

            id             |                                                   blob_uri                                                   | file_type |        file_name         |          created_at
---------------------------+--------------------------------------------------------------------------------------------------------------+-----------+--------------------------+-------------------------------
 blob.9c6a2be6135288dc141a | az://reports/afcc58f1-486c-437c-9fce-0a4774cd4128/index.html                                                 | html      | index.html               | 2024-01-12 22:24:07.695008+00
 blob.47508626ad7c784151d6 | az://reports/afcc58f1-486c-437c-9fce-0a4774cd4128/libs/accessible-code-block-0.0.1/empty-anchor.js           | js        | empty-anchor.js          | 2024-01-12 22:24:07.695008+00
 blob.e952045a95b60b0744fd | az://reports/afcc58f1-486c-437c-9fce-0a4774cd4128/libs/anchor-sections-1.1.0/anchor-sections-hash.css        | css       | anchor-sections-hash.css | 2024-01-12 22:24:07.695008+00
 blob.59b3c81bad6b701bbf83 | az://reports/afcc58f1-486c-437c-9fce-0a4774cd4128/libs/anchor-sections-1.1.0/anchor-sections.js              | js        | anchor-sections.js       | 2024-01-12 22:24:07.695008+00
 blob.7edd40c453bac1c53754 | az://reports/afcc58f1-486c-437c-9fce-0a4774cd4128/libs/gitbook-2.6.7/css/fontawesome/fontawesome-webfont.ttf | ttf       | fontawesome-webfont.ttf  | 2024-01-12 22:24:07.695008+00
 blob.7eda957bd02cbf3ab01f | az://reports/afcc58f1-486c-437c-9fce-0a4774cd4128/libs/gitbook-2.6.7/css/plugin-search.css                   | css       | plugin-search.css        | 2024-01-12 22:24:07.695008+00

Serving these files will be a follow-up PR

This PR makes assorted tweaks to the runner's `create-report` task, like: - Adding new `pacta.FileType`s for assorted HTML-adjacent types present in the report (see below) - Includes an `UNKNOWN` type in case I miss anything, we can always go back and re-label them based on file extension - Fix some issues with paths, use the portfolio UUID as the directory prefix (to avoid collisions) - Publish the event when we're done so the main app can record the info Mostly unrelated changes: - Fixes #135 by pulling `handleMissingTranslation` from the `$nuxt` object instead of `inject` - Add a helper script for iterating quickly on the runner, `build_and_load_runner.sh` Some resources that get created from this: ``` select * from blob WHERE blob_uri LIKE 'az://reports/%'; id | blob_uri | file_type | file_name | created_at ---------------------------+--------------------------------------------------------------------------------------------------------------+-----------+--------------------------+------------------------------- blob.9c6a2be6135288dc141a | az://reports/afcc58f1-486c-437c-9fce-0a4774cd4128/index.html | html | index.html | 2024-01-12 22:24:07.695008+00 blob.47508626ad7c784151d6 | az://reports/afcc58f1-486c-437c-9fce-0a4774cd4128/libs/accessible-code-block-0.0.1/empty-anchor.js | js | empty-anchor.js | 2024-01-12 22:24:07.695008+00 blob.e952045a95b60b0744fd | az://reports/afcc58f1-486c-437c-9fce-0a4774cd4128/libs/anchor-sections-1.1.0/anchor-sections-hash.css | css | anchor-sections-hash.css | 2024-01-12 22:24:07.695008+00 blob.59b3c81bad6b701bbf83 | az://reports/afcc58f1-486c-437c-9fce-0a4774cd4128/libs/anchor-sections-1.1.0/anchor-sections.js | js | anchor-sections.js | 2024-01-12 22:24:07.695008+00 blob.7edd40c453bac1c53754 | az://reports/afcc58f1-486c-437c-9fce-0a4774cd4128/libs/gitbook-2.6.7/css/fontawesome/fontawesome-webfont.ttf | ttf | fontawesome-webfont.ttf | 2024-01-12 22:24:07.695008+00 blob.7eda957bd02cbf3ab01f | az://reports/afcc58f1-486c-437c-9fce-0a4774cd4128/libs/gitbook-2.6.7/css/plugin-search.css | css | plugin-search.css | 2024-01-12 22:24:07.695008+00 ```

gbdubs

Woo!

gbdubs · 2024-01-13T00:20:59Z

scripts/build_and_load_runner.sh

@@ -0,0 +1,20 @@
+#!/bin/bash
+set -euo pipefail


I LOVE THIS IT WILL MAKE LIFE EASIER THANK YOU

HAPPY TO HELP YEAH I WASNT ENJOYING DOING THIS MANUALLY EITHER

gbdubs · 2024-01-13T00:22:03Z

pacta/pacta.go

+	FileType_CSS,
+	FileType_JS,
+	FileType_TTF,
+	FileType_UNKNOWN,


Nit: I don't think we want unknown in this list, since it's used to test things like convertibility to postgres, and we don't want "" to be persistable.

^^ discussed above

gbdubs · 2024-01-13T00:22:16Z

frontend/plugins/handle-missing-translation.ts

  const values = computed(() => {
    return missingTranslations.value
  })
  return {
    provide: {
      missingTranslations: {
+        handleMissingTranslation,


Love this, great, elegant solution.

frontend/i18n.config.ts

gbdubs · 2024-01-13T00:23:18Z

db/sqldb/golden/schema_dump.sql

+    'css',
+    'js',
+    'ttf',
+    ''


Let's not add '' as a valid enum type, we don't want to persist it.

Discussed in the comment above, I think we do. I'm also open to calling this unknown or whatever you think makes sense here

gbdubs · 2024-01-13T00:24:02Z

db/sqldb/blob.go

 	if b.FileName == "" {
 		return fmt.Errorf("blob missing FileName")
 	}
+	// A blank FileType is valid, just means we don't recognize the file type yet.


I'd disagree with that - we require a file type to be set, and I think that's a pretty straightforward invariant to maintain for the course of development. Is there a reason to support this?

The context here is that the create-report action generates a whole directory of files, including HTML, JSON, CSS, JavaScript, fonts, and text files. I imagine the PACTA folks will want to modify these reports over time, potentially introducing new files and resources that should be served as part of the report.

If one of those new files is of a type we haven't enumerated (videos? various image formats? XML files? etc), we still want to persist the file and make a best-effort attempt to serve it. My solution is to say "well, we didn't recognize the type of the file at the time of report creation", which is UNKNOWN.

Happy to chat more if that doesn't make sense or you have another preferred approach.

Yeah I guess I'm not sold there. In my mind the value of an enum is confidence you'll get back a value you expect. I think if we (at root) don't trust the data we have coming in to be of the type(s) that we expect, and we don't have a need to handle them specifically, we should simply remove the enum constraint generally, and store it as a string-based file extension.

I don't want to throw the baby out with the bathwater here. In most cases (currently all), the blob is one of a small list of types. It's just that the alternative when we encounter an unknown extension is to ignore the file entirely, which is definitely not the option we want.

And the file type enum is more generally useful than a string extension. For example, yml and yaml are both YAML files

Added this check back, as UNKNOWN is now 'unknown'

gbdubs · 2024-01-13T00:25:05Z

cmd/runner/main.go

+			return fmt.Errorf("given blob wasn't a JSON-formatted portfolio, %q", fileNameWithExt)
+		}
+		fileNames = append(fileNames, strings.TrimSuffix(fileNameWithExt, ".json"))
+		destPath := filepath.Join("/", "mnt", "processed_portfolios", fileNameWithExt)


https://tldp.org/LDP/Linux-Filesystem-Hierarchy/html/mnt.html

Basically, /mnt is a generic place for external things being mounted in. It's a convention Alex used in the stub repo, and it makes as much sense as any other location.

gbdubs · 2024-01-13T00:26:26Z

cmd/runner/README.md

@@ -15,11 +15,12 @@ bazel run //scripts:run_server -- --use_azure_runner

 ### Creating a new docker image to run locally

+When testing locally (e.g. without `--use_azure_runner`), you can build and tag a runner image locally and use that. To do that, run `bazel run //scripts:build_and_load_runner`


This leaves ambiguous what happens if you're not using either of these options? Could you make that a bit clearer?

GC, made this section more explicit

bcspragu added 4 commits January 12, 2024 14:38

Run DB regen

f82cc85

Update migrations test

72b2143

Fix SQL test

0c1b96b

bcspragu requested a review from gbdubs January 12, 2024 23:52

gbdubs approved these changes Jan 13, 2024

View reviewed changes

bcspragu added 2 commits January 12, 2024 19:49

Address review comments

69a9def

Rename '' filetype to 'unknown'

2352743

bcspragu merged commit df77dc4 into main Jan 16, 2024
2 checks passed

bcspragu deleted the brandon/create-report branch January 16, 2024 02:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get the `create-report` async task in shape #138

Get the `create-report` async task in shape #138

bcspragu commented Jan 12, 2024 •

edited

Loading

gbdubs left a comment

gbdubs Jan 13, 2024

bcspragu Jan 13, 2024

gbdubs Jan 13, 2024

bcspragu Jan 13, 2024

gbdubs Jan 13, 2024

gbdubs Jan 13, 2024

bcspragu Jan 13, 2024

gbdubs Jan 13, 2024

bcspragu Jan 13, 2024

gbdubs Jan 13, 2024

bcspragu Jan 13, 2024

bcspragu Jan 16, 2024

gbdubs Jan 13, 2024

bcspragu Jan 13, 2024

gbdubs Jan 13, 2024

gbdubs Jan 13, 2024

bcspragu Jan 13, 2024

		@@ -15,11 +15,12 @@ bazel run //scripts:run_server -- --use_azure_runner

		### Creating a new docker image to run locally

		When testing locally (e.g. without `--use_azure_runner`), you can build and tag a runner image locally and use that. To do that, run `bazel run //scripts:build_and_load_runner`

Get the create-report async task in shape #138

Get the create-report async task in shape #138

Conversation

bcspragu commented Jan 12, 2024 • edited Loading

gbdubs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Get the `create-report` async task in shape #138

Get the `create-report` async task in shape #138

bcspragu commented Jan 12, 2024 •

edited

Loading