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

CSS-4739 Removes the dashboard resource. #996

Merged
merged 1 commit into from
Jul 11, 2023

Conversation

alesstimec
Copy link
Collaborator

Description

JIMM will no longer be serving the dashboard files since we have a juju dashboard charm that serves the dashboard. The other alternative is for JIMM to redirect the user to the location that serves the dashboard.

Engineering checklist

Check only items that apply

  • Documentation updated
  • Covered by unit tests
  • Covered by integration tests

Test instructions

Notes for code reviewers

JIMM will no longer be serving the dashboard files since we have a juju dashboard
charm.
@alesstimec alesstimec changed the title CSS-4744 Removes the dashboard resource. CSS-4739 Removes the dashboard resource. Jul 11, 2023
Copy link
Contributor

@kian99 kian99 left a comment

Choose a reason for hiding this comment

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

LGTM, I had a look at the integration tests to see whether any mention of the dashboard needed to be removed from there, but doesn't seem like it.

Copy link
Member

@babakks babakks left a comment

Choose a reason for hiding this comment

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

Just one question.

@@ -40,126 +32,8 @@ func Handler(ctx context.Context, loc string, publicDNSname string) http.Handler
}
if u != nil && u.IsAbs() {
mux.Handle(dashboardPath, http.RedirectHandler(loc, http.StatusPermanentRedirect))
return mux
} else {
zapctx.Warn(ctx, "not redirecting to the dashboard", zap.String("dashboard_location", loc))
Copy link
Member

Choose a reason for hiding this comment

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

Now that we don't serve local files, what happens when we get a relative path? It seems we do nothing here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

correct..

@alesstimec alesstimec merged commit 27339bc into canonical:main Jul 11, 2023
Copy link
Contributor

@ale8k ale8k left a comment

Choose a reason for hiding this comment

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

As a lot is removed, I'm gonna say yes but not 100% if all is removed

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.

4 participants