support new `Url` config fields #577

miles-grant-ibigroup · 2023-11-20T16:14:57Z

Checklist

Appropriate branch selected (all PRs must first be merged to dev before they can be merged to master)
Any modified or new methods or classes have helpful JavaDoc and code is thoroughly commented
The description lists all applicable issues this PR seeks to resolve
The description lists any configuration setting(s) that differ from the default settings
All tests and CI builds passing
The description lists all relevant PRs included in this release (remove this if not merging to master)
e2e tests are all passing (remove this if not merging to master)

Description

Adds 2 new fields to Deployment that allow for downloading the router/build config from a URL. If the URL field is present, the old field is ignored, and a new file downloaded instead. Downloaded file is parsed only by Java's bulit-in toString method, so safety-wise we should be ok.

Looking for feedback

Should mongo be updated when we download the data? Are errors handled well? How should we test this?

philip-cline

Looks fine to me, two small questions

src/main/java/com/conveyal/datatools/manager/models/Deployment.java

philip-cline · 2023-11-20T16:40:05Z

src/main/java/com/conveyal/datatools/manager/models/Deployment.java

@@ -453,6 +492,16 @@ private <O extends Serializable> String writeToString(O object) {
    public byte[] generateRouterConfig() throws IOException {
        Project project = this.parentProject();

+        if (this.customRouterConfigUrl != null) {
+            try {
+                // TODO: should Mongo be updated here?


Should this comment be added above the line for this.customBuildConfig as well?

The question is targeted at you. Is leaving the comment the best bet do you think?

If not here, where will it be updated?

Well I think in this line instead of updating the local instantiated Deployment we'd update Mongo

Right I see what you're saying now. Updating the mongo might be more in line with what we do currently elsewhere

…d test):

br648 · 2023-11-21T12:11:02Z

@miles-grant-ibigroup I've made a few code changes, quicker than going back and forth through comments. I hope you don't mind.

I've added a test, it might not be what you are looking for, but will at least test that the download from URL works.

I'm not sure on saving to Mongo. How often will the downloaded config change and how often will it be download?! If you save the config to Mongo, you then have to deal with the logic around downloading a newer config. We can discuss this if you want?

miles-grant-ibigroup · 2023-11-21T12:14:24Z

@miles-grant-ibigroup I've made a few code changes, quicker than going back and forth through comments. I hope you don't mind.

I've added a test, it might not be what you are looking for, but will at least test that the download from URL works.

I'm not sure on saving to Mongo. How often will the downloaded config change and how often will it be download?! If you save the config to Mongo, you then have to deal with the logic around downloading a newer config. We can discuss this if you want?

Thanks for your refactor I for the most part much prefer it! However, does this take into account that if no URL is set the buildConfig/routerConfig from Mongo should be used?

br648 · 2023-11-21T14:16:41Z

@miles-grant-ibigroup I've made a few code changes, quicker than going back and forth through comments. I hope you don't mind.
I've added a test, it might not be what you are looking for, but will at least test that the download from URL works.
I'm not sure on saving to Mongo. How often will the downloaded config change and how often will it be download?! If you save the config to Mongo, you then have to deal with the logic around downloading a newer config. We can discuss this if you want?

Thanks for your refactor I for the most part much prefer it! However, does this take into account that if no URL is set the buildConfig/routerConfig from Mongo should be used?

I've made a small change to only assign the downloaded config if available.

miles-grant-ibigroup · 2023-11-21T14:33:44Z

@miles-grant-ibigroup I've made a few code changes, quicker than going back and forth through comments. I hope you don't mind.
I've added a test, it might not be what you are looking for, but will at least test that the download from URL works.
I'm not sure on saving to Mongo. How often will the downloaded config change and how often will it be download?! If you save the config to Mongo, you then have to deal with the logic around downloading a newer config. We can discuss this if you want?

Thanks for your refactor I for the most part much prefer it! However, does this take into account that if no URL is set the buildConfig/routerConfig from Mongo should be used?

I've made a small change to only assign the downloaded config if available.

This looks great thanks so much! Things are working for me but would appreciate if someone else gave it a quick check too to make sure it still works

philip-cline · 2023-11-21T15:15:08Z

I can give it a quick check

br648

LGTM.

philip-cline

Did some more testing on this, it seems to be working well. I had two small comments on the new code as well.

philip-cline · 2023-11-20T17:59:09Z

src/main/java/com/conveyal/datatools/manager/models/Deployment.java

@@ -453,6 +492,16 @@ private <O extends Serializable> String writeToString(O object) {
    public byte[] generateRouterConfig() throws IOException {
        Project project = this.parentProject();

+        if (this.customRouterConfigUrl != null) {
+            try {
+                // TODO: should Mongo be updated here?


Right I see what you're saying now. Updating the mongo might be more in line with what we do currently elsewhere

src/main/java/com/conveyal/datatools/manager/models/Deployment.java

support new Url config fields

b72cff3

miles-grant-ibigroup assigned philip-cline Nov 20, 2023

miles-grant-ibigroup changed the title ~~support new Url config fields~~ support new Url config fields Nov 20, 2023

philip-cline approved these changes Nov 20, 2023

View reviewed changes

philip-cline assigned miles-grant-ibigroup and unassigned philip-cline Nov 20, 2023

miles-grant-ibigroup assigned br648 and unassigned miles-grant-ibigroup Nov 20, 2023

miles-grant-ibigroup mentioned this pull request Nov 20, 2023

allow URL type for router and build config ibi-group/datatools-ui#1001

Merged

refactor(Refactored the config download aspect of Deployment and adde…

b3210a8

…d test):

refactor(Deployment.java): Refactor downloaded config assignment.

a2cbede

refactor(Removed unused imports.):

f142def

br648 approved these changes Nov 21, 2023

View reviewed changes

br648 assigned philip-cline and unassigned br648 Nov 21, 2023

philip-cline approved these changes Nov 23, 2023

View reviewed changes

philip-cline assigned miles-grant-ibigroup and unassigned philip-cline Nov 23, 2023

address pr feedback

8357af0

miles-grant-ibigroup merged commit 272dbd3 into dev Nov 27, 2023
5 checks passed

miles-grant-ibigroup deleted the download-otp-configs-from-url branch November 27, 2023 14:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support new `Url` config fields #577

support new `Url` config fields #577

miles-grant-ibigroup commented Nov 20, 2023 •

edited

Loading

philip-cline left a comment

philip-cline Nov 20, 2023

miles-grant-ibigroup Nov 20, 2023

philip-cline Nov 20, 2023

miles-grant-ibigroup Nov 20, 2023

philip-cline Nov 20, 2023

br648 commented Nov 21, 2023

miles-grant-ibigroup commented Nov 21, 2023

br648 commented Nov 21, 2023

miles-grant-ibigroup commented Nov 21, 2023

philip-cline commented Nov 21, 2023

br648 left a comment

philip-cline left a comment

philip-cline Nov 20, 2023

support new Url config fields #577

support new Url config fields #577

Conversation

miles-grant-ibigroup commented Nov 20, 2023 • edited Loading

Checklist

Description

Looking for feedback

philip-cline left a comment

Choose a reason for hiding this comment

philip-cline Nov 20, 2023

Choose a reason for hiding this comment

miles-grant-ibigroup Nov 20, 2023

Choose a reason for hiding this comment

philip-cline Nov 20, 2023

Choose a reason for hiding this comment

miles-grant-ibigroup Nov 20, 2023

Choose a reason for hiding this comment

philip-cline Nov 20, 2023

Choose a reason for hiding this comment

br648 commented Nov 21, 2023

miles-grant-ibigroup commented Nov 21, 2023

br648 commented Nov 21, 2023

miles-grant-ibigroup commented Nov 21, 2023

philip-cline commented Nov 21, 2023

br648 left a comment

Choose a reason for hiding this comment

philip-cline left a comment

Choose a reason for hiding this comment

philip-cline Nov 20, 2023

Choose a reason for hiding this comment

support new `Url` config fields #577

support new `Url` config fields #577

miles-grant-ibigroup commented Nov 20, 2023 •

edited

Loading