Serve UI in trustify #176

carlosthe19916 · 2024-04-17T10:52:21Z

@bobmcwhirter you mentioned the UI being served by the trustd server. Here is a working example of it.

The current PR allows the UI to be served by trustd but the UI can still live alone without the need of trustd (e.g a container of only the UI can be created). So the UI can live in standalone mode but also within trustd.
This PR implies moving back https://github.com/trustification/trustify-ui to this repository. I don't know if that is a good idea, in the past there were complains about it therefore the UI was moved to its own repository. What is your opinion?

I opened this PR mainly to show your idea in action. I'll defer the decision of where the UI code should live. So no need to merge this PR until we are happy with it.

How to test this PR

Checkout this PR
Make sure you have NodeJS 20 installed and npm >9.5.0
Execute:

cargo run --bin trustd -- --auth-disabled

Open http://localhost:8080

bobmcwhirter · 2024-04-17T11:53:07Z

I love the idea but I suspect we can find a way so you can keep UI in your own repo. I'll ponder a bit.

ctron · 2024-04-17T12:54:48Z

I think it's great to see that this is within reach! I really love that.

When it comes to having the code in the same repository, then the only thing that was bothering me in the past was the fact that there had been numerous warnings spamming the CI logs. So making a change on the backend side brought up a lot of unrelated warnings from the UI side. If that's solved (and stays solved in general) then I think it would be good to have both parts in the same repository. But that is just me. I understand people having a different preference.

bobmcwhirter · 2024-04-17T14:53:55Z

So, you know I'm a monorepo kinda guy.

@carlosthe19916 -- what's your view on where it should live? Do you have a strong opinion? If merged back in, can we ensure that CI logs are quite clean?

carlosthe19916 · 2024-04-17T18:46:53Z

So, you know I'm a monorepo kinda guy.

@carlosthe19916 -- what's your view on where it should live? Do you have a strong opinion? If merged back in, can we ensure that CI logs are quite clean?

I personally have no preference on where the code lives.
I do have a preference in terms of being able to merge my own PRs in the ui/ directory. Not sure what configuration to put in place in Github so I would be able to merge PRs without approvals (only on the ui directory). The other option is that the team could be willing to review UI code continuously so I don't wait long periods of time before getting things done. Could we relax the rules for the ui directory? If possible, let's move the UI here :)

ctron · 2024-04-18T06:41:35Z

I do have a preference in terms of being able to merge my own PRs in the ui/ directory. Not sure what configuration to put in place in Github so I would be able to merge PRs without approvals (only on the ui directory). The other option is that the team could be willing to review UI code continuously so I don't wait long periods of time before getting things done. Could we relax the rules for the ui directory? If possible, let's move the UI here :)

I think that points a big flaw. I am ok with just pushing to main or even force pushing every now and then. However, that shouldn't be the norm. We are actually required to have a review process in place by our internal policies. And for good reason!

If we do not have enough team members to do even a basic review, then I think we have a problem. Just ignoring that problem by not doing reviews feels completely wrong.

bobmcwhirter · 2024-04-18T11:09:53Z

Would @gildub be a suitable reviewer? He has some react experience yeah?

carlosthe19916 · 2024-04-18T11:48:19Z

Anyone can be a reviewer, the only requirement is the wiliness to do it.
So guys, my only desire is to get things done fast enough. And I am 100% accountable for what is written in the UI. Honestly, I'm afraid of once we merge the UI into this repository discussions/arguments out of scope might be raised and distract us from our daily tasks and slow things down.

What do you think @gildub , would you be willing to review UI PRs? I'd rather ask you rather than give you a task without your permission :)

PhilipCattanach · 2024-04-18T13:41:08Z

@carlosthe19916 The PR review process is a very well established best practice that we must continue to do. As we discussed this morning we need to get more Engineers capable of contributing to the UI. @gildub already has those skills so he should be your go to guy for now.

jcrossley3 · 2024-04-18T13:47:38Z

From the commentary, it seems obvious we should keep the two repos separate for now. Everyone has mentioned potential downsides to doing this, and the only barely recognizable upside I see is "you know I'm a monorepo kinda guy".

bobmcwhirter · 2024-04-18T13:51:30Z

@jcrossley3 well, the original motivation was "having trustd" serve it. But, I also think we can sort that across repos, somehow.

gildub · 2024-04-22T17:26:23Z

I think a monorepo approach makes sense for a project organized with full stack teams working across the board (likely broken down by services) otherwise having a separate repo is easier to maintain by an UI dedicated team.
So don't we need to discuss which way we'd like to work ?

In the meantime, I can review UI PRs.

helio-frota · 2024-04-29T13:34:17Z

otherwise having a separate repo is easier to maintain by an UI dedicated team.

In the meantime, I can review UI PRs.

I volunteer to also review UI 👍

carlosthe19916 · 2024-05-07T20:25:22Z

@bobmcwhirter @jcrossley3 @ctron I have updated this PR so the UI code still lives in its own repository.
There is still more things to do but this should be a good enough first attempt to have the PM mode working (PM mode does not have authentication).

To test this PR you only need to run

cargo run --bin trustd

Just like it is a requisite to have Rust installed, it is also a requisite to have NodeJs 20 installed in your computer. You can use https://github.com/nvm-sh/nvm for installing NodeJS.

jcrossley3 · 2024-05-07T20:41:15Z

Sorry, but I'm an idiot, and I require more hand-holding. Can you include specific commands at the top of the README for seeing the UI? All I see is "Error" when I visit http://localhost:8080. You can assume I have git and nvm installed, but what idempotent commands can I run to ensure I actually see something when I try for the first time?

You can fill a barn with what I don't know about git submodules and nvm. My ui/ directory is empty, so I'm sure I'm missing something.

bobmcwhirter · 2024-05-07T20:42:28Z

Ah submodules strike again!

jcrossley3 · 2024-05-07T21:12:16Z

I don't think I want this. I'd prefer to treat the two repos separately. Serving the UI from the api server is a cool deployment feature, and I'm down for adding whatever tooling is necessary for packaging that, but IMO it's an overall negative for development.

If I get outvoted, we need to at least fix build.rs so that it doesn't think it needs to build trustd even when no source changed.

jcrossley3 · 2024-05-07T21:18:01Z

trustd/build.rs

@@ -34,7 +34,9 @@ fn main() {
 }

 fn clone_ui() -> io::Result<ExitStatus> {
-    Command::new(GIT_CMD).args(["submodule", "update"]).status()
+    Command::new(GIT_CMD)
+        .args(["submodule", "update", "--init", "--recursive"])


Ah! I figured this out on my own, not realizing you were attempting to prevent me from having to do that.

yep, the idea is not requiring the user to execute additional commands

This change fixed my issue, thanks!

carlosthe19916 · 2024-05-07T21:20:27Z

Sorry, but I'm an idiot, and I require more hand-holding. Can you include specific commands at the top of the README for seeing the UI? All I see is "Error" when I visit http://localhost:8080. You can assume I have git and nvm installed, but what idempotent commands can I run to ensure I actually see something when I try for the first time?

You can fill a barn with what I don't know about git submodules and nvm. My ui/ directory is empty, so I'm sure I'm missing something.

I have added some minor changes to the README.md file
The idea of this PR is for the user to need only 1 command to start everything, and that command should be cargo run --bin trustd. If that is not working, then that was not this PR's goal and I should change "something" for making it work.

I know you already confirmed you have NVM but could you double check you have NodeJS 20?

node -v
v20.11.1

In theory you should not even need to clone the submodules as it should be done by the code here https://github.com/trustification/trustify/pull/176/files#diff-d6cffee37c35a86ae367b378bde922d3a28bd32512b9d5ed3bb81ffba7027492R37

@jcrossley3 you spotted a mistake on my side. The command for cloning the git submodule was not correct at all, I fixed it in my last commits

Sorry for the mistake, but could you test it again please?

carlosthe19916 · 2024-05-07T21:43:44Z

but IMO it's an overall negative for development.

I agree.

jcrossley3

I still don't think I want this, but if we must, we need to bring the log output back.

jcrossley3 · 2024-05-07T21:40:26Z

trustd/build.rs

@@ -34,7 +34,9 @@ fn main() {
 }

 fn clone_ui() -> io::Result<ExitStatus> {
-    Command::new(GIT_CMD).args(["submodule", "update"]).status()
+    Command::new(GIT_CMD)
+        .args(["submodule", "update", "--init", "--recursive"])


This change fixed my issue, thanks!

jcrossley3 · 2024-05-07T21:42:04Z

trustd/build.rs

+use std::{fs, io};
+
+static UI_DIR: &str = "../ui";
+static UI_DIR_SRC: &str = "../ui/src";


Suggested change

static UI_DIR_SRC: &str = "../ui/src";

This path doesn't exist, so cargo builds each time.

jcrossley3 · 2024-05-07T21:42:51Z

trustd/build.rs

+fn main() {
+    println!("Build Trustify - build.rs!");
+
+    println!("cargo:rerun-if-changed={}", UI_DIR_SRC);


Suggested change

println!("cargo:rerun-if-changed={}", UI_DIR_SRC);

println!("cargo:rerun-if-changed={}", UI_DIR);

This is what you meant, I think, and cargo only builds when something beneath there changes.

bobmcwhirter · 2024-05-07T21:48:34Z

Let's see if we can't crate it like the swagger UI and use as a dependency instead of git shenanigans. Say shenanigans one more time.

bobmcwhirter · 2024-05-07T21:49:43Z

Then random jackholes like me won't need NPM.

jcrossley3 · 2024-05-07T21:56:39Z

Then random jackholes like me won't need NPM.

You're expecting those same random jackholes to install rustup, correct? nvm is the rustup analog, and both seem fairly jackhole-proof to me.

jcrossley3 · 2024-05-07T21:57:47Z

But I agree, a crate would be nice.

bobmcwhirter · 2024-05-07T22:13:10Z

Yah I'm wrong. Source means we'd all still compile it locally including NPM.

Which is still better than submodules.

ctron · 2024-05-08T07:26:29Z

I think this is getting a little bit out of hand.

What about just packaging a .zip file during the build of the UI. We could even automate this in the CI (of the UI). Maybe even using tags and GitHub released for once (we can use pre-releases there too).

And then just fetch, add, commit, and push that zip (or it's extracted content) to this repository.

Assuming we do this via something like a "makefile", where there's a single "version" field which refreshes this, we could even run this in the CI and ensure that there are no local changes that we missed (like we did it for the Helm Charts to OpenShift templates conversion).

bobmcwhirter · 2024-05-09T17:56:27Z

Closing, solved differently.

carlosthe19916 requested a review from bobmcwhirter April 17, 2024 10:52

carlosthe19916 force-pushed the ui branch from 125a2cc to 2ad367b Compare April 17, 2024 10:56

carlosthe19916 added 6 commits May 7, 2024 21:07

Move UI to trustify

a2e2e60

Add actions/setup-node to the ci workflow

0175571

Make clippy happy

ee3189e

Fix ui build instructions

0b0a475

remove ui folder

6a08728

include UI code through git submodules

0616bf6

carlosthe19916 force-pushed the ui branch from 341ee40 to 0616bf6 Compare May 7, 2024 20:00

Include code for cloning ui

a299161

carlosthe19916 requested review from jcrossley3, ctron and bobmcwhirter and removed request for bobmcwhirter May 7, 2024 20:20

Update README.md

105fff4

Fix git submodule clone command

938fb92

jcrossley3 reviewed May 7, 2024

View reviewed changes

jcrossley3 requested changes May 7, 2024

View reviewed changes

carlosthe19916 mentioned this pull request May 8, 2024

Add instructions for running UI using the container image #249

Merged

bobmcwhirter closed this May 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Serve UI in trustify #176

Serve UI in trustify #176

carlosthe19916 commented Apr 17, 2024

bobmcwhirter commented Apr 17, 2024

ctron commented Apr 17, 2024

bobmcwhirter commented Apr 17, 2024

carlosthe19916 commented Apr 17, 2024

ctron commented Apr 18, 2024

bobmcwhirter commented Apr 18, 2024

carlosthe19916 commented Apr 18, 2024

PhilipCattanach commented Apr 18, 2024

jcrossley3 commented Apr 18, 2024 •

edited

Loading

bobmcwhirter commented Apr 18, 2024

gildub commented Apr 22, 2024 •

edited

Loading

helio-frota commented Apr 29, 2024

carlosthe19916 commented May 7, 2024

jcrossley3 commented May 7, 2024

bobmcwhirter commented May 7, 2024

jcrossley3 commented May 7, 2024

jcrossley3 May 7, 2024

carlosthe19916 May 7, 2024

jcrossley3 May 7, 2024

carlosthe19916 commented May 7, 2024

carlosthe19916 commented May 7, 2024 •

edited

Loading

jcrossley3 left a comment

jcrossley3 May 7, 2024

jcrossley3 May 7, 2024

jcrossley3 May 7, 2024

bobmcwhirter commented May 7, 2024

bobmcwhirter commented May 7, 2024

jcrossley3 commented May 7, 2024

jcrossley3 commented May 7, 2024

bobmcwhirter commented May 7, 2024

ctron commented May 8, 2024

bobmcwhirter commented May 9, 2024

	println!("cargo:rerun-if-changed={}", UI_DIR_SRC);
	println!("cargo:rerun-if-changed={}", UI_DIR);

Serve UI in trustify #176

Serve UI in trustify #176

Conversation

carlosthe19916 commented Apr 17, 2024

How to test this PR

bobmcwhirter commented Apr 17, 2024

ctron commented Apr 17, 2024

bobmcwhirter commented Apr 17, 2024

carlosthe19916 commented Apr 17, 2024

ctron commented Apr 18, 2024

bobmcwhirter commented Apr 18, 2024

carlosthe19916 commented Apr 18, 2024

PhilipCattanach commented Apr 18, 2024

jcrossley3 commented Apr 18, 2024 • edited Loading

bobmcwhirter commented Apr 18, 2024

gildub commented Apr 22, 2024 • edited Loading

helio-frota commented Apr 29, 2024

carlosthe19916 commented May 7, 2024

jcrossley3 commented May 7, 2024

bobmcwhirter commented May 7, 2024

jcrossley3 commented May 7, 2024

jcrossley3 May 7, 2024

Choose a reason for hiding this comment

carlosthe19916 May 7, 2024

Choose a reason for hiding this comment

jcrossley3 May 7, 2024

Choose a reason for hiding this comment

carlosthe19916 commented May 7, 2024

carlosthe19916 commented May 7, 2024 • edited Loading

jcrossley3 left a comment

Choose a reason for hiding this comment

jcrossley3 May 7, 2024

Choose a reason for hiding this comment

jcrossley3 May 7, 2024

Choose a reason for hiding this comment

jcrossley3 May 7, 2024

Choose a reason for hiding this comment

bobmcwhirter commented May 7, 2024

bobmcwhirter commented May 7, 2024

jcrossley3 commented May 7, 2024

jcrossley3 commented May 7, 2024

bobmcwhirter commented May 7, 2024

ctron commented May 8, 2024

bobmcwhirter commented May 9, 2024

jcrossley3 commented Apr 18, 2024 •

edited

Loading

gildub commented Apr 22, 2024 •

edited

Loading

carlosthe19916 commented May 7, 2024 •

edited

Loading