Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Workspaces UI #38

Merged
merged 9 commits into from
Aug 26, 2024
Merged

Conversation

cloud-work535
Copy link
Collaborator

@cloud-work535 cloud-work535 commented Aug 19, 2024

Adding new ui image to the AWS templates. ALB templates are not supported and I have to rollback the ALB template changes. ALB templates will be removed in another PR before the release date.

@cloud-work535 cloud-work535 self-assigned this Aug 19, 2024
@cloud-work535
Copy link
Collaborator Author

all-in-one-without-lb:

image

ai-unlimited-without-lb

image

all-in-one-nlb

image

ai-unilimited-with-nlb

image

@cloud-work535 cloud-work535 marked this pull request as ready for review August 23, 2024 14:34
@johandry
Copy link

johandry commented Aug 23, 2024

The variable AiUnlimitedHttpPort should be named AiUnlimitedAuthPort because this is not more for HTTP.
If you do the change, make sure to rename it everywhere.

Same with the description: port to access the AI Unlimited UI should be port to access the AI Unlimited Authentication Service

Copy link

@johandry johandry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are basically these changes:

  • Use AI Unlimited UI instead of Workspaces UI for the services name and description
  • Use the port variables for HTTP (to be renamed) and gRPC instead of hardcoded values
  • Use the UI Healthcheck URL, although there is a typo that will be fixed
  • Rename the HTTP Port variable name and description to Auth Port

@@ -461,6 +487,49 @@ Resources:
docker:
enabled: "true"
ensureRunning: "true"
configure_workspaces_ui_service:
files:
/usr/lib/systemd/system/workspaces-ui.service:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be ai-unlimited-ui.service instead workspaces-ui.service

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I was changing the ui name from "workspaces-ui" to "ai-unlimited-ui" and version on my next commit. waiting for the image to be ready and UI fix to test and push them all at once.

/usr/lib/systemd/system/workspaces-ui.service:
content: !Sub |
[Unit]
Description=workspaces-ui

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be AI Unlimited UI instead of workspaces-ui

Comment on lines 516 to 517
-e TD_VCD_API_PORT=3282 \
-e TD_VCD_AUTH_PORT=3000 \

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the HTTP and gRPC Port are a variables, these ports should be too.

-e TD_VCD_AUTH_PORT=${ AiUnlimitedHttpPort } \
-e TD_VCD_API_PORT=${ AiUnlimitedGrpcPort }  \

start_workspaces_ui_service:
services:
systemd:
workspaces-ui:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, should be ai-unlimited-ui instead of workspaces-ui

Value: !Sub http://${ LoadBalancer.DNSName }:${ AiUnlimitedHttpPort }
Value: !If
- PortIsNotEighty
- !Sub "http://${ LoadBalancer.DNSName }:${ AiUnlimitedUiPort }/landing"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The UI have a health check too. Use "http://${ LoadBalancer.DNSName }:${ AiUnlimitedUiPort }/healtcheck.

There is a typo, that I will change in a PR this week, should be /healthcheck 😄

@@ -479,6 +505,49 @@ Resources:
docker:
enabled: "true"
ensureRunning: "true"
configure_workspaces_ui_service:
files:
/usr/lib/systemd/system/workspaces-ui.service:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment about using UI Unlimited instead of Workspaces

/usr/lib/systemd/system/workspaces-ui.service:
content: !Sub |
[Unit]
Description=workspaces-ui

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment about using UI Unlimited instead of Workspaces

group: root
mode: "000400"
owner: root
start_workspaces_ui_service:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment about using UI Unlimited instead of Workspaces

start_workspaces_ui_service:
services:
systemd:
workspaces-ui:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment about using UI Unlimited instead of Workspaces

Value: !Sub http://${ LoadBalancer.DNSName }:${ AiUnlimitedHttpPort }
Value: !If
- PortIsNotEighty
- !Sub "http://${ LoadBalancer.DNSName }:${ AiUnlimitedUiPort }/landing"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment about using the UI Healthcheck

Comment on lines +534 to +535
-e TD_VCD_API_PORT=3282 \
-e TD_VCD_AUTH_PORT=3000 \

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API and Auth port hardcoded, should use the variables.

@cloud-work535 cloud-work535 changed the base branch from develop to feature/add-workspaces-ui-rebase-2 August 26, 2024 19:52
@cloud-work535 cloud-work535 merged commit 5c6c691 into feature/add-workspaces-ui-rebase-2 Aug 26, 2024
1 check failed
@cloud-work535 cloud-work535 deleted the workspaces-ui branch August 26, 2024 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants