Added vite config and updated deploy to gh-pages script. #102

SamTV12345 · 2024-02-20T07:53:29Z

No description provided.

SamTV12345 · 2024-02-20T07:54:01Z

I already tested the build locally and it works with your current configuration. So you don't need to change any gh-pages settings.

anuraaga · 2024-02-20T08:05:40Z

@SamTV12345 Doh, sorry I forgot I am able to push to your branch, I had meant to push to my fork and send a PR to this one. I found having the duplicate files in the same repo to be a bit confusing (gh-pages had a nice aspect of shielding that to some extent). I figured it could be worth to have vite do the copy, and confirmed it with built and dev server. How does it look? If not good, feel free to revert, sorry for accidentally pushing

08f96d6

SamTV12345 · 2024-02-20T08:23:16Z

@anuraaga Great idea. I just updated the license information but everything else looks great.

codefromthecrypt

quick notes. thanks for getting this started!

can you change the configuration to reference the yaml proto and thrift from their original locations instead of copying them to the public directory?
- we shouldn't need to change the locations of these files, and probably some have saved links to the source tree also. copies of files is generally bad in git anyway
can you add update .github/workflows to make sure this build works on commit?

codefromthecrypt · 2024-02-20T11:28:45Z

actually the github actions test is already configured to run the root npm commands. We shouldn't have a mix of build tools in the same repo. Can you raise a different PR to make the root project use the same infra you are using here? you can update /pom.xml too to make sure it is downloading the correct stuff (run ./mvnw test to verify)

SamTV12345 · 2024-02-20T11:31:20Z

actually the github actions test is already configured to run the root npm commands. We shouldn't have a mix of build tools in the same repo. Can you raise a different PR to make the root project use the same infra you are using here? you can update /pom.xml too to make sure it is downloading the correct stuff (run ./mvnw test to verify)

Sure. I'll do this in around 6 hours.

codefromthecrypt · 2024-02-20T11:33:37Z

so basically in the other pr, update /package* for the switch off npm (also need to update here https://github.com/openzipkin/zipkin-api/blob/master/pom.xml#L181). Please don't remove maven or anything else, mainly we are just setting on one tool for npm-like stuff. Then, we can rebase this PR over that and the instructions won't be like.. use npm if in the root folder, or if in the docs, use pnpm.

Also, I think this project directory should be named swagger or openapi as that's what it is building. I don't think there's any proto or thrift integration for swagger anyway right?

codefromthecrypt · 2024-02-20T11:35:41Z

docs/README.md

@@ -0,0 +1,14 @@
+# Zipkin API


I think this README belongs where it was, at the root of the project, sam,e for the other files.

The only thing we publish to gh-pages is the swagger stuff

Oh. I think this landed on the pages branch unexpectedly. I just put everything into the public folder.

ok maybe after digesting the feedback take a close look at files changed etc. the way I do reviews as you are starting to notice is.. evertyihng is liability until it is proven needed ;) so let's try to get what you've accomplished into the smallest diff with the smallest amount of changes. mention me when ready for another pass, and meanwhile, thanks for the help!

codefromthecrypt · 2024-02-20T11:36:58Z

maybe the best name for this directory is 'site'? I think docs is ok, but doesn't hint that it is published and maybe that's the most important part about it.

codefromthecrypt · 2024-02-20T11:40:23Z

last and most important comment for the night.

remember, the only reason why we are adding machinery here is if it can fix the XSS. Please make a comment what part of this fixes the XSS. If the result of this doesn't fix the XSS we lost the problem and started doing something else entirely. We have to be problem focused, not infra focused, as more infra means more things that can break.

codefromthecrypt · 2024-02-20T11:44:54Z

here's the XSS thing #98

I was under the impression we were making a custom build to fix that, so basically if it doesn't fix that I really don't want a custom build, as it would end up me maintaining this when people lose interest.

SamTV12345 · 2024-02-20T11:46:03Z

here's the XSS thing #98

I was under the impression we were making a custom build to fix that, so basically if it doesn't fix that I really don't want a custom build, as it would end up me maintaining this when people lose interest.

It does fix XSS. I built the api and entered the query parameter as shown in the example. It doesn't show an alert window or a different swagger api yaml

codefromthecrypt · 2024-02-20T11:56:54Z

great! glad it fixes it. In the future, put the fix verification into the PR desc to remove fear uncertainty and doubt (FUD) like I had.

next: what's most uncomfortable is that instead of modifying the existing javascript build (/package.json which verifies the swagger), we created a new project copying some of the files and making a completely different build. What I was expecting was a modification to the original build, not a new directoty where root tests the swagger and another project with different tooling publishes it.

Can you look into flattening this? modify the existing build, so no two projects which gives a problem of how to test both individually and what to name the thing that publishes what the other one tests?

SamTV12345 · 2024-02-21T13:21:08Z

great! glad it fixes it. In the future, put the fix verification into the PR desc to remove fear uncertainty and doubt (FUD) like I had.

next: what's most uncomfortable is that instead of modifying the existing javascript build (/package.json which verifies the swagger), we created a new project copying some of the files and making a completely different build. What I was expecting was a modification to the original build, not a new directoty where root tests the swagger and another project with different tooling publishes it.

Can you look into flattening this? modify the existing build, so no two projects which gives a problem of how to test both individually and what to name the thing that publishes what the other one tests?

Done. I changed the config of the frontend eirslett plugin and moved the tests to the site folder.

codefromthecrypt · 2024-02-22T00:17:54Z

site/public/thrift/zipkinDependencies.thrift

@@ -0,0 +1,39 @@
+#


seems like this is still copying IDL (thrift, openapi and proto) from the prior location. By flattening I meant to make this the root project (so no site). Does that help avoid copying or moving these files from their current location?

I'm not sure if this is beneficial. If I'd like to check out the yaml myself I would had to know that this is hidden in a subfolder under side.

this won't merge with copies of files in the same repo as it subverts the purpose of git.

there will be a few trends you'll notice here which apply to this decision

don't break existing people by default

there are certainly links to the files in the original locations

don't make copies of the gold copy

there is source history on the original files, and making copies of them confuses that

only one file of the copied would be tested or distributed

we prefer retaining the experience for the many vs the few

as you noticed swagger is updated like every few years, and by almost no one

we wouldn't break any of the above rules especially for this case

hope the rationale helps. If you really don't want to make this refer to other files, because you feel strongly, I can add a commit to do the same, so it wouldn't have your name on it.

codefromthecrypt · 2024-02-23T00:41:13Z

site/public/zipkin.proto

@@ -0,0 +1,14 @@
+//
+// Copyright 2018-2024 The OpenZipkin Authors


also I don't know why we are pushing this and the thrift to the swagger site, as they haven't anything to do with swagger. Maybe simplest to bring this PR back to swagger, as that's only about zipkin-api.yaml.

When we do much more than requested, we end up with more decisions to make and that slows down change. Let's focus on the problem which was only about swagger and only about XSS. Theres' no relevance to the proto and thrift, so let's keep it out of this so we can land the fix?

codefromthecrypt

concrete steps forward

please flatten the project to root, so there is no more site directory, and people (or CI) don't accidentally skip validating the yaml.
remove thrift and proto from the public folder
if you can't access zipkin-api.yaml from the current directory, add a symlink as not supporting windows is better than 2 copies of the same file.

Finally, let's get through this as there are a lot more interesting things to accomplish together, and we can spend the same amount of iterations on them! The more time I repeat the same things, the more time both of us lose and the more time the community loses to things I can work on alternatively.

codefromthecrypt

getting closer to least required change. thanks for getting this closer.

last mile is less copy/paste a.k.a. less undifferentiated labor for the next person.

If we have a custom build *and also copy/paste" it calls into question why we don't just unzip the swagger UI like we did before and make POST impossible vs this, which is a lot more elaborate.

Consider the next person maintaining this may have zero frontend experience and before this change they needed none, and now they will struggle. Let's make the struggle the least and also do least copy/paste as it is burdensome to cite in LICENSE and also give instructions on when to re-copy/paste.

codefromthecrypt · 2024-03-03T00:27:46Z

public/404.html

@@ -0,0 +1,152 @@
+<!--


I don't think we need such an elaborate 404. it calls into question if it was copy/pasted etc. I don't think anyone here will be able to maintain it. Let's use a simple (no css) or default please?

codefromthecrypt · 2024-03-03T00:29:12Z

public/oauth2-redirect.html

+    the License.
+
+-->
+<!doctype html>


was this copied from something? Also why do we need oauth of any kind as we aren't authenticating this? If we must maintain a custom build of openapi, seems we don't need to add things few will be able to maintain and also might require LICENSE notices.

codefromthecrypt · 2024-03-03T00:29:37Z

src/style.css

+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */


empty file? let's delete this

codefromthecrypt · 2024-03-03T00:30:11Z

src/vite-env.d.ts

+/// the License.
+///
+
+/// <reference types="vite/client" />


this appears to be commented out. unless I'm missing something, please delete it. If I am missing something, add a comment why it is commented out?

codefromthecrypt · 2024-03-03T00:31:10Z

vite.config.ts

+                    dest: '',
+                },
+                {
+                    src: normalizePath(resolve(__dirname, './thrift')),


why are we copying these files which aren't used by swagger? let's not expand the change beyond fixing the XSS in swagger?

#102 (comment) is why. I don't know if comments are possible here, but lets add them as we moved responsibility for something that seems orthogonal unless you knew it.

codefromthecrypt

notes on pnpm, which I'm fine with as long as it is consistent.

codefromthecrypt · 2024-03-03T01:40:48Z

pom.xml

@@ -36,10 +36,11 @@

    <!-- Update occasionally based on LTS, but don't overrun what's in Alpine:
         https://pkgs.alpinelinux.org/packages?name=nodejs-current -->


Suggested change

https://pkgs.alpinelinux.org/packages?name=nodejs-current -->

https://pkgs.alpinelinux.org/packages?name=nodejs-current

https://pkgs.alpinelinux.org/packages?name=pnpm -->

codefromthecrypt · 2024-03-03T01:42:09Z

pom.xml

@@ -36,10 +36,11 @@

    <!-- Update occasionally based on LTS, but don't overrun what's in Alpine:
         https://pkgs.alpinelinux.org/packages?name=nodejs-current -->
-    <node.version>14.14.0</node.version>
+    <node.version>21.6.2</node.version>
+    <pnpm.version>8.15.3</pnpm.version>


alpine doesn't have this version yet.

Suggested change

<pnpm.version>8.15.3</pnpm.version>

<pnpm.version>8.15.1</pnpm.version>

Later when we do the same in zipkin, we have to update the script accordingly https://github.com/openzipkin/zipkin/blob/master/build-bin/maybe_install_npm

tl;dr; is only way we should change the build tool here is if we also change it in zipkin (including any README citations) because otherwise it is harder or more annoying when context switching.

codefromthecrypt · 2024-03-03T01:45:17Z

ps there's a comment out-of-date in build-bin test. Whenever changing a build tool, use ag, grep or your favorite search tool to make sure all references including doc comments are not saying something out of date.

codefromthecrypt

notes about the push script

codefromthecrypt · 2024-03-03T01:46:18Z

build-bin/dist_to_gh_pages

 # Update gh-pages
-git fetch origin gh-pages:gh-pages
-git checkout gh-pages


let me know how you tested this script as historically changes to scripts like this break and I need to fix them later when deployment failed.

codefromthecrypt · 2024-03-03T01:49:11Z

build-bin/dist_to_gh_pages


-# TODO: these files can be in a list for convenience.


maybe this is why you were adding this in the vite-config? This deserves a comment there to avoid me re-asking the same questions!

something like?

// these files were formerly checked out from master in build-bin/idl_to_gh_pages. // We now package them directly when making the openapi dist

codefromthecrypt · 2024-03-03T01:49:27Z

build-bin/dist_to_gh_pages

-    echo "Pushing ${files_to_commit} to gh-pages"
-    git push origin gh-pages
+    git add --all
+    git commit -m "Automatically updated dist"


Suggested change

git commit -m "Automatically updated dist"

git commit -m "Automatically updated site"

codefromthecrypt · 2024-03-03T01:54:00Z

build-bin/dist_to_gh_pages

@@ -15,26 +15,37 @@

 set -ue

+ORIGIN_BRANCH='master'


these variables add confusion. I guess you are extracting them for tests. Please use these conventions if we must override for tests and add a comment that we are doing this.

# we never change these during deployment, these variables are only here for ad-hoc tests. DEFAULT_BRANCH=master GH_PAGES_BRANCH=gh-pages

codefromthecrypt · 2024-03-03T01:57:21Z

build-bin/dist_to_gh_pages

+# Remove everything except .git
+rm -rf *
+
+git checkout $ORIGIN_BRANCH docs


The less vocabulary in a script, the better. I was wondering what this "docs" directory was about. Consider the name of the repo is zipkin-api and there is no directory named docs anywhere. I was looking at this script later and wondering if something made a directory named docs via the build process.

In our docker builds, we use the directory named "install" that works, workspace, etc.

Suggested change

git checkout $ORIGIN_BRANCH docs

git checkout $DEFAULT_BRANCH workspace

codefromthecrypt · 2024-03-03T02:03:42Z

the reason this simple affect (stop the XSS) is taking many hours over several weeks, is that many things are combined into the same change. This adds labor as a lot of things have to be adjusted, and the important parts are held captive to unimportant parts.

For example, as you know right now, I really am not keen on everything here as it is much more work than before, not just during review, but forever.

The change says "Added vite config and updated deploy to gh-pages script."
The goal was to remove the XSS

The change here also does things that steal review time and could be far simpler done independently in different PRs
Like this:

change the current build to pnpm
change the validation script to typescript
use vite build for openapi instead of swagger (yes this updates the gh-pages script)
change non-swagger api publication from script to vite

codefromthecrypt · 2024-03-03T02:09:56Z

@SamTV12345 last comment is something we can avoid next time. Small changes at the same time as something related are one thing, but this changes a lot of things at once. This has taken many hours of my time, and really I don't even like the result vs overwriting the html. I do like parts of it! If something like the vite part fails later and we have to revert, we have to also revert the good parts. In general, I use the term "holding code hostage" when there's a PR that has very independent easy to say yes things mixed in with difficult parts. While I will continue to work with you to get this landed, let's please not do this next time, as I keep missing family time over things like this.

codefromthecrypt · 2024-03-03T02:16:44Z

I'm going to do max one more review passes on this. If things aren't mergable I have to set a personal boundary and close the PR. I would then make the script to remove the XSS and add README instructions how to make sure it is disabled. As a project lead, I can't spend so much time on things that are not required to solve a problem. cc @reta @anuraaga @shakuzen as until someone else triple checks things, it is just me doing this, and I'm getting burnt out on it.

codefromthecrypt · 2024-03-03T02:31:44Z

unless this is added to pom.xml we won't know the build is broken until master tries to deploy

--- a/pom.xml
+++ b/pom.xml
@@ -217,6 +217,17 @@
               <arguments>run test</arguments>
             </configuration>
           </execution>
+          <execution>
+            <id>pnpm run build</id>
+            <goals>
+              <goal>pnpm</goal>
+            </goals>
+            <phase>package</phase>
+            <configuration>
+              <arguments>run build</arguments>
+            </configuration>
+          </execution>
         </executions>
       </plugin>

codefromthecrypt · 2024-03-03T02:33:59Z

build-bin/dist_to_gh_pages

@@ -15,26 +15,37 @@



note renaming this file will break deploy. you can fix that by also changing build-bin/deploy with the name change.

In general, unless using an IDE like IntelliJ which detects this sort of thing for you, go back and do a recursive search any time you rename or delete some part of the build.

codefromthecrypt · 2024-03-03T03:16:17Z

Unless someone else can reproduce it, I think we already fixed #98 when we updated the swagger dist recently. I think I must have not refreshed my browser after #101

Unless someone can reproduce this (I have tried several browsers including mobile), let's step back and not keep moving forward with a custom UI.

https://zipkin.io/zipkin-api/?configUrl=data:text/html;base64,ewoidXJsIjoiaHR0cHM6Ly9leHViZXJhbnQtaWNlLnN1cmdlLnNoL3Rlc3QueWFtbCIKfQ==

changes below are welcome, but let's not dig a hole with making our own UI dist. If anything, we can setup a GH action to download and update our gh-pages content if it is really necessary, but meanwhile I will raise a PR to update docs.

change the current build to pnpm
change the validation script to typescript
nothing more than this, please!

Added vite config and updated deploy to gh-pages script.

bb7378c

Use vite-plugin-static-copy to copy files

08f96d6

SamTV12345 added 2 commits February 20, 2024 09:21

Updated license

fbec90a

Updated vite config

1ae72c4

codefromthecrypt reviewed Feb 20, 2024

View reviewed changes

Fixed cli tests.

39de04b

codefromthecrypt reviewed Feb 22, 2024

View reviewed changes

Don't install peer deps.

7bf756b

codefromthecrypt reviewed Feb 23, 2024

View reviewed changes

Flattened directory structure.

f45a102

codefromthecrypt reviewed Mar 3, 2024

View reviewed changes

codefromthecrypt closed this Mar 3, 2024

		@@ -0,0 +1,14 @@
		//
		// Copyright 2018-2024 The OpenZipkin Authors

		@@ -36,10 +36,11 @@

		<!-- Update occasionally based on LTS, but don't overrun what's in Alpine:
		https://pkgs.alpinelinux.org/packages?name=nodejs-current -->

	<pnpm.version>8.15.3</pnpm.version>
	<pnpm.version>8.15.1</pnpm.version>

	git commit -m "Automatically updated dist"
	git commit -m "Automatically updated site"

	git checkout $ORIGIN_BRANCH docs
	git checkout $DEFAULT_BRANCH workspace

Added vite config and updated deploy to gh-pages script. #102

Added vite config and updated deploy to gh-pages script. #102

Conversation

SamTV12345 commented Feb 20, 2024

SamTV12345 commented Feb 20, 2024

anuraaga commented Feb 20, 2024 • edited Loading

SamTV12345 commented Feb 20, 2024

codefromthecrypt left a comment

Choose a reason for hiding this comment

codefromthecrypt commented Feb 20, 2024 • edited Loading

SamTV12345 commented Feb 20, 2024

codefromthecrypt commented Feb 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codefromthecrypt commented Feb 20, 2024

codefromthecrypt commented Feb 20, 2024

codefromthecrypt commented Feb 20, 2024

SamTV12345 commented Feb 20, 2024

codefromthecrypt commented Feb 20, 2024

SamTV12345 commented Feb 21, 2024

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

codefromthecrypt left a comment

Choose a reason for hiding this comment

codefromthecrypt 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

codefromthecrypt 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

codefromthecrypt commented Mar 3, 2024

codefromthecrypt 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

codefromthecrypt commented Mar 3, 2024 • edited Loading

codefromthecrypt commented Mar 3, 2024

codefromthecrypt commented Mar 3, 2024

codefromthecrypt commented Mar 3, 2024 • edited Loading

Choose a reason for hiding this comment

codefromthecrypt commented Mar 3, 2024

anuraaga commented Feb 20, 2024 •

edited

Loading

codefromthecrypt commented Feb 20, 2024 •

edited

Loading

codefromthecrypt commented Mar 3, 2024 •

edited

Loading

codefromthecrypt commented Mar 3, 2024 •

edited

Loading