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

General fixes after QA #988

Merged
merged 1 commit into from
Jul 7, 2023

Conversation

alesstimec
Copy link
Contributor

Description

  • fix for the dashboard relation: keys using _ instead of -
  • properly rendering config.js for 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

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 without enough context on some of the changes.

for i := range dbCloud.Regions {
dbCloud.Regions[i].Controllers = []dbmodel.CloudRegionControllerPriority{{
ControllerID: region.Controllers[0].ID,
Priority: dbmodel.CloudRegionControllerPrioritySupported,
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
Contributor 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

@@ -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
Contributor 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

- fix for the dashboard relation: keys using _ instead of -
- properly rendering config.js for the dashboard
@alesstimec alesstimec merged commit 73c3379 into canonical:main Jul 7, 2023
7 checks passed
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.

2 participants