Skip to content

Deployment template dashboard#49

Merged
aaronchongth merged 11 commits intomainfrom
fix/paths
Mar 20, 2025
Merged

Deployment template dashboard#49
aaronchongth merged 11 commits intomainfrom
fix/paths

Conversation

@aaronchongth
Copy link
Copy Markdown
Member

@aaronchongth aaronchongth commented Feb 26, 2025

Bug fix

Fixed bug

Fix issues mentioned in https://github.com/open-rmf/rmf/discussions/599#

  • inject variables into correct folder, broken since d2638d4
  • Fix paths to resources
  • robot icon scale
  • ingress for trajectory server

Screenshot from 2025-03-06 17-06-42

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
@aaronchongth aaronchongth changed the title Inject envs into correct files Deployment template dashboard Feb 27, 2025
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
@aaronchongth aaronchongth marked this pull request as ready for review March 6, 2025 09:11
Copy link
Copy Markdown

@koonpeng koonpeng left a comment

Choose a reason for hiding this comment

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

main should never be used as the commit to deploy, it should always be a commit hash. I notice that the rmf dockerfiles are using main as well, so this is a bigger problem outside the scope of this PR, I will open an issue regarding this.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There shouldn't be a separate local and with_auth images (there shouldn't be a local version). These 2 version are also out of sync, some settings like defaultRobotZoom are different.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I've fixed the difference for now. As discussed with @akash-roboticist, since this is a template for reference and local testing, not for deployment, we will just add more information into the README to remind users to change things up when we address #50.

Copy link
Copy Markdown

@koonpeng koonpeng Mar 11, 2025

Choose a reason for hiding this comment

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

Can you explain the use case for the local version? iiuc, "local testing" means a deployment into a local kubernetes cluster so there should be no reason to ever use the version without auth.

Copy link
Copy Markdown
Member Author

@aaronchongth aaronchongth Mar 17, 2025

Choose a reason for hiding this comment

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

@koonpeng it's for local testing via docker-compose, https://github.com/open-rmf/rmf_deployment_template/blob/main/devel/docker-compose-local.yaml, I think it is a good intermediate step for quick testing, before setting up an entire cluster

edit: it's mentioned in the README, but hidden under the expand

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I still don't think it should be there, a docker compose deploy is very different from a k8s deploy, even if we still want to "test" a deployment by running a different deployment, the docker compose should still have authentication, database, logging, reverse proxy etc.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This part of the deployment template was set up since quite a long time ago from one of our deployment projects. What we do with it, I'd revert to @akash-roboticist then

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@koonpeng , like @aaronchongth mentioned, the idea for the barebones docker-compose was to enable users to test things rapidly while in dev, without needing the deeper understanding of prod systems or spending the time to setup a full cluster.
A concrete example is third party equipment devs when testing their equipment need a sandbox environment prior to integrating with a real setup. It would be too much of an ask for such devs if the setup involves more than running a docker-compose up command.
So, perhaps we could re-frame the conversation as - what would be the alternative / enhancement needed to the existing docker-compose method, to enable such a throwaway deployment, without raising the setup complexity?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think it comes down to scope. imo, the main issue is that the deployment (template) shouldn't include a different dashboard with no auth that must never be used in an actual deployment. I think the use case of a "stub" rmf seems more suited to be in another repo like rmf_demos, or at least be isolated in a directory in this repo. Actually, I noticed there is already a devel directory for this purpose (which is where the docker-compose live), so then the minimum we should do is to move the "no auth" dashboard out of the deployment template and into this directory, that would make it more clear that this dashboard is not representative of the deployment template.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This sounds reasonable, thanks for your input!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Just made the changes according to this discussion, e92de0a

…oard configs

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
@aaronchongth aaronchongth requested a review from koonpeng March 10, 2025 14:48
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
@aaronchongth aaronchongth requested a review from koonpeng March 11, 2025 06:09
Signed-off-by: Aaron Chong <aaronchong@google.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Copy link
Copy Markdown
Member

@akash-roboticist akash-roboticist left a comment

Choose a reason for hiding this comment

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

LGTM

@aaronchongth aaronchongth merged commit 890615b into main Mar 20, 2025
1 check passed
@aaronchongth aaronchongth deleted the fix/paths branch March 20, 2025 02:21
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.

3 participants