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.

Sign up for GitHub

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

Jump to bottom

General fixes after QA #988

Merged
merged 1 commit into from
Jul 7, 2023
Merged

General fixes after QA #988

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions charms/jimm-k8s/src/charm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,9 +290,9 @@ def _update_workload(self, event):
if dashboard_relation and self.unit.is_leader():
dashboard_relation.data[self.app].update(
{
"controller-url": "wss://{}".format(dns_name),
"identity-provider-url": self.config.get("candid-url"),
"is-juju": str(False),
"controller_url": "wss://{}".format(dns_name),
"identity_provider_url": self.config.get("candid-url"),
"is_juju": str(False),
}
)

Expand All @@ -319,9 +319,9 @@ def _on_dashboard_relation_joined(self, event: RelationJoinedEvent):

event.relation.data[self.app].update(
{
"controller-url": "wss://{}".format(dns_name),
"identity-provider-url": self.config["candid-url"],
"is-juju": str(False),
"controller_url": "wss://{}".format(dns_name),
"identity_provider_url": self.config["candid-url"],
"is_juju": str(False),
}
)

Expand Down
6 changes: 3 additions & 3 deletions charms/jimm-k8s/tests/unit/test_charm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,11 +253,11 @@ def test_dashboard_relation_joined(self):

self.assertTrue(data)
self.assertEqual(
data["controller-url"],
data["controller_url"],
"wss://juju-jimm-k8s-0.juju-jimm-k8s-endpoints.None.svc.cluster.local",
)
self.assertEqual(data["identity-provider-url"], "https://candid.example.com")
self.assertEqual(data["is-juju"], "False")
self.assertEqual(data["identity_provider_url"], "https://candid.example.com")
self.assertEqual(data["is_juju"], "False")

@patch("src.charm.JimmOperatorCharm._get_network_address")
@patch("socket.gethostname")
Expand Down
6 changes: 3 additions & 3 deletions charms/jimm/src/charm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,9 +386,9 @@ def _snap(self, *args):
def _on_dashboard_relation_joined(self, event):
event.relation.data[self.app].update(
{
"controller-url": "wss://{}".format(self.config["dns-name"]),
"identity-provider-url": self.config["candid-url"],
"is-juju": str(False),
"controller_url": "wss://{}".format(self.config["dns-name"]),
"identity_provider_url": self.config["candid-url"],
"is_juju": str(False),
}
)

Expand Down
6 changes: 3 additions & 3 deletions charms/jimm/tests/test_charm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,9 +488,9 @@ def test_dashboard_relation_joined(self):
harness.add_relation_unit(id, "juju-dashboard/0")
data = harness.get_relation_data(id, "juju-jimm")
self.assertTrue(data)
self.assertEqual(data["controller-url"], "wss://jimm.example.com")
self.assertEqual(data["identity-provider-url"], "https://candid.example.com")
self.assertEqual(data["is-juju"], "False")
self.assertEqual(data["controller_url"], "wss://jimm.example.com")
self.assertEqual(data["identity_provider_url"], "https://candid.example.com")
self.assertEqual(data["is_juju"], "False")


class VersionHTTPRequestHandler(BaseHTTPRequestHandler):
Expand Down
3 changes: 2 additions & 1 deletion internal/dashboard/dashboard.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ const (
// handler will redirect to that URL. If the location is a path on the disk
// then the handler will serve the files from that path. Otherwise a
// NotFoundHandler will be returned.
func Handler(ctx context.Context, loc string) http.Handler {
func Handler(ctx context.Context, loc string, publicDNSname string) http.Handler {
mux := http.NewServeMux()

u, err := url.Parse(loc)
Expand Down Expand Up @@ -85,6 +85,7 @@ func Handler(ctx context.Context, loc string) http.Handler {
continue
}
var w bytes.Buffer
configParams["baseControllerURL"] = publicDNSname
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we weren't rendering the config.js properly.. the controller url was set to None due to a bug in the dashboard and jimm

if err := t.Execute(&w, configParams); err != nil {
zapctx.Error(ctx, "error executing config.js.go", zap.Error(err))
continue
Expand Down
16 changes: 8 additions & 8 deletions internal/dashboard/dashboard_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import (
const (
configFile = `var jujuDashboardConfig = {
// API host to allow app to connect and retrieve models
baseControllerURL: null,
baseControllerURL: "{{.baseControllerURL}}",
// Configurable base url to allow deploying to different paths.
baseAppURL: "{{.baseAppURL}}",
// If true then identity will be provided by a third party provider.
Expand All @@ -38,7 +38,7 @@ const (
func TestDashboardNotConfigured(t *testing.T) {
c := qt.New(t)

hnd := dashboard.Handler(context.Background(), "")
hnd := dashboard.Handler(context.Background(), "", "")
rr := httptest.NewRecorder()
req, err := http.NewRequest("GET", "/dashboard", nil)
c.Assert(err, qt.IsNil)
Expand All @@ -50,7 +50,7 @@ func TestDashboardNotConfigured(t *testing.T) {
func TestDashboardRedirect(t *testing.T) {
c := qt.New(t)

hnd := dashboard.Handler(context.Background(), "https://example.com/dashboard")
hnd := dashboard.Handler(context.Background(), "https://example.com/dashboard", "http://jimm.canonical.com")
rr := httptest.NewRecorder()
req, err := http.NewRequest("GET", "/dashboard", nil)
c.Assert(err, qt.IsNil)
Expand All @@ -71,7 +71,7 @@ func TestDashboardFromPath(t *testing.T) {
err = os.WriteFile(filepath.Join(dir, "test"), []byte(testFile), 0444)
c.Assert(err, qt.Equals, nil)

hnd := dashboard.Handler(context.Background(), dir)
hnd := dashboard.Handler(context.Background(), dir, "http://jimm.canonical.com")
rr := httptest.NewRecorder()
req, err := http.NewRequest("GET", "/dashboard", nil)
c.Assert(err, qt.IsNil)
Expand Down Expand Up @@ -100,7 +100,7 @@ func TestDashboardFromPath(t *testing.T) {
c.Assert(err, qt.IsNil)
c.Check(string(buf), qt.Equals, `var jujuDashboardConfig = {
// API host to allow app to connect and retrieve models
baseControllerURL: null,
baseControllerURL: "http://jimm.canonical.com",
// Configurable base url to allow deploying to different paths.
baseAppURL: "/",
// If true then identity will be provided by a third party provider.
Expand Down Expand Up @@ -136,7 +136,7 @@ func TestDashboardFromPath(t *testing.T) {
func TestInvalidLocation(t *testing.T) {
c := qt.New(t)

hnd := dashboard.Handler(context.Background(), ":::")
hnd := dashboard.Handler(context.Background(), ":::", "http://jimm.canonical.com")
rr := httptest.NewRecorder()
req, err := http.NewRequest("GET", "/dashboard", nil)
c.Assert(err, qt.IsNil)
Expand All @@ -152,7 +152,7 @@ func TestLocationNotDirectory(t *testing.T) {
err := os.WriteFile(filepath.Join(dir, "test"), []byte(testFile), 0444)
c.Assert(err, qt.Equals, nil)

hnd := dashboard.Handler(context.Background(), filepath.Join(dir, "test"))
hnd := dashboard.Handler(context.Background(), filepath.Join(dir, "test"), "http://jimm.canonical.com")
rr := httptest.NewRecorder()
req, err := http.NewRequest("GET", "/dashboard", nil)
c.Assert(err, qt.IsNil)
Expand All @@ -174,7 +174,7 @@ func TestGUIArchiveEndpoint(t *testing.T) {
err = os.WriteFile(filepath.Join(dir, "version.json"), []byte(versionFile), 0444)
c.Assert(err, qt.Equals, nil)

hnd := dashboard.Handler(context.Background(), dir)
hnd := dashboard.Handler(context.Background(), dir, "http://jimm.canonical.com")
rr := httptest.NewRecorder()
req, err := http.NewRequest("GET", "/gui-archive", nil)
c.Assert(err, qt.IsNil)
Expand Down
4 changes: 3 additions & 1 deletion internal/jimm/cloud.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,12 +393,14 @@ func (j *JIMM) AddHostedCloud(ctx context.Context, u *dbmodel.User, tag names.Cl
}
// Update the cloud in the database.
dbCloud.FromJujuCloud(*ccloud)
zapctx.Debug(ctx, "received cloud info from controller", zap.Any("cloud", dbCloud))
for i := range dbCloud.Regions {
dbCloud.Regions[i].Controllers = []dbmodel.CloudRegionControllerPriority{{
ControllerID: region.Controllers[0].ID,
ControllerID: controller.ID,
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels significant, can you explain what it was doing and why it's changing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

because this never worked.. notice line 395, where we get information from the controller on the added cloud.. well.. that information does not contain the controller ID.. and this update was failing due to foreign key constraints

Priority: dbmodel.CloudRegionControllerPrioritySupported,
}}
}
zapctx.Debug(ctx, "received cloud info from controller", zap.Any("cloud", dbCloud))

if err := j.Database.UpdateCloud(ctx, &dbCloud); err != nil {
// At this point the cloud has been created on the
Expand Down
2 changes: 1 addition & 1 deletion service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ func NewService(ctx context.Context, p Params) (*Service, error) {
// If the request is not for a known path assume it is part of the dashboard.
// If dashboard location env var is not defined, do not handle a dashboard.
if p.DashboardLocation != "" {
s.mux.Handle("/", dashboard.Handler(ctx, p.DashboardLocation))
s.mux.Handle("/", dashboard.Handler(ctx, p.DashboardLocation, p.PublicDNSName))
}

return s, nil
Expand Down