ibi-group / datatools-server

Server for IBI's GTFS data management platform.
MIT License
49 stars 51 forks source link

support new `Url` config fields #577

Closed miles-grant-ibigroup closed 9 months ago

miles-grant-ibigroup commented 9 months ago

Checklist

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?

br648 commented 9 months ago

@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 commented 9 months ago

@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 commented 9 months ago

@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 commented 9 months ago

@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 commented 9 months ago

I can give it a quick check