More auto-deploy changes. #381

evansiroky · 2021-05-07T07:10:34Z

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

Description

A few more changes so that auto-deployment works somewhat smoothly in practice. And a refactor to consolidate a bunch of job-related logic into its own util file.

Move all job-related fields and methods into new JobUtils class.
Introduce a new active field into MonitorableJob to correctly identify active jobs even when jobs are recurring or not yet started.
Change the way active jobs are found to use that new active field.
When queueing a new DeployJob, wait to create a new one so it doesn't show up in list of jobs for a split second.
Add new field autoDeployWithCriticalErrors to Project and update AutoDeployJob to use this field accordingly
Add a bunch of new completion messages to the AutoDeployJob
Only update feed versions in a deployment when the AutoDeployJob starts a DeployJob
Avoid overwriting deployment data in DeployJob by fetching needed data at point of update

codecov-commenter · 2021-05-07T07:13:31Z

Codecov Report

Merging #381 (c4d04c7) into auto-deploy (3083c5f) will decrease coverage by 0.08%.
The diff coverage is 24.59%.

@@                Coverage Diff                @@
##             auto-deploy     #381      +/-   ##
=================================================
- Coverage          28.94%   28.85%   -0.09%     
- Complexity           729      734       +5     
=================================================
  Files                183      184       +1     
  Lines               9388     9405      +17     
  Branches            1281     1281              
=================================================
- Hits                2717     2714       -3     
- Misses              6384     6407      +23     
+ Partials             287      284       -3

Flag	Coverage Δ	Complexity Δ
unit_tests	`28.85% <24.59%> (-0.09%)`	`734.00 <12.00> (+5.00)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
...ols/editor/controllers/api/SnapshotController.java	`15.21% <0.00%> (ø)`	`2.00 <0.00> (ø)`
...va/com/conveyal/datatools/manager/ConvertMain.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...va/com/conveyal/datatools/manager/DataManager.java	`75.51% <ø> (-0.37%)`	`36.00 <0.00> (ø)`
.../datatools/manager/controllers/DumpController.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
.../manager/controllers/api/DeploymentController.java	`9.00% <0.00%> (-2.58%)`	`2.00 <0.00> (-1.00)`
.../manager/controllers/api/FeedSourceController.java	`34.50% <0.00%> (ø)`	`9.00 <0.00> (ø)`
...manager/controllers/api/FeedVersionController.java	`12.33% <0.00%> (ø)`	`2.00 <0.00> (ø)`
...ls/manager/controllers/api/GtfsPlusController.java	`6.61% <0.00%> (ø)`	`2.00 <0.00> (ø)`
...ols/manager/controllers/api/ProjectController.java	`13.19% <0.00%> (ø)`	`2.00 <0.00> (ø)`
...ools/manager/controllers/api/StatusController.java	`25.00% <0.00%> (+3.57%)`	`2.00 <0.00> (-3.00)`	⬆️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3083c5f...c4d04c7. Read the comment docs.

br648

This looks fine to me.

landonreed · 2021-05-07T12:45:14Z

src/main/java/com/conveyal/datatools/manager/controllers/api/DeploymentController.java

     */
-    public static boolean queueDeployJob(DeployJob deployJob) {
-        String serverId = deployJob.getServerId();
+    public static DeployJob queueDeployJob(Deployment deployment, Auth0UserProfile owner, OtpServer server) {


Seems like this method and the deploymentJobsByServer might now belong in JobUtils also?

Refactored in c4d04c7

landonreed · 2021-05-07T12:59:53Z

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

+    /**
+     * Whether to auto-deploy feeds that have critical errors.
+     */
+    public boolean autoDeployWithCriticalErrors = false;


Is this a field that we are going to add into the UI or is it just for testing?

Yes, see ibi-group/datatools-ui#637

landonreed

Just a couple of minor comments, but this all seems to make sense!

binh-dam-ibigroup

LGTM with some minor comments.

binh-dam-ibigroup · 2021-05-07T14:10:12Z

src/main/java/com/conveyal/datatools/manager/jobs/AutoDeployJob.java

+            project.pinnedDeploymentId == null ||
+                deployment == null ||
+                deployment.feedVersionIds.isEmpty() ||
+                lockedProjects.contains(project.id)


I prefer to have the conditions indented the same, but that may be an IntelliJ thing.

binh-dam-ibigroup · 2021-05-07T14:13:27Z

src/main/java/com/conveyal/datatools/manager/jobs/AutoDeployJob.java

            if (latestVersionsWithCriticalErrors.size() > 0) {
                StringBuilder errorMessageBuilder = new StringBuilder(
-                    String.format("Auto deployment skipped for project %s! %s feed(s) contain critical errors:",
+                    String.format("Auto deployment for project %s! %s feed(s) contain critical errors:",


Why was 'skipped' removed? The resulting text is an incomplete sentence.

The deployment is now not always skipped if there are feeds with critical errors if the autoDeployWithCriticalErrors is set to true. Incomplete sentence fixed in c4d04c7

binh-dam-ibigroup · 2021-05-07T14:36:13Z

src/main/java/com/conveyal/datatools/manager/jobs/DeployJob.java

+            // newer feed versions exist! Start a new auto-deploy job.
+            JobUtils.heavyExecutor.execute(new AutoDeployJob(deployment.parentProject(), owner));
+        } else {
+            LOG.info("No need to start another auto-deployment");


I am tempted to say the log should be when we start another auto-deployment.

binh-dam-ibigroup · 2021-05-07T15:05:16Z

@br648 I'm assigning this back to you because this PR is against #361 that you created. You can coordinate any changes needed with @evansiroky prior to merging.

See #381

refactor(auto-deploy): refactor a few things to improve auto-deployment

db462d0

evansiroky assigned landonreed, br648 and binh-dam-ibigroup May 7, 2021

evansiroky mentioned this pull request May 7, 2021

Add OTP auto-deploy project setting ibi-group/datatools-ui#637

Merged

5 tasks

br648 approved these changes May 7, 2021

View reviewed changes

br648 removed their assignment May 7, 2021

landonreed reviewed May 7, 2021

View reviewed changes

landonreed approved these changes May 7, 2021

View reviewed changes

landonreed assigned evansiroky and unassigned landonreed May 7, 2021

binh-dam-ibigroup approved these changes May 7, 2021

View reviewed changes

binh-dam-ibigroup assigned br648 and unassigned binh-dam-ibigroup May 7, 2021

refactor: address PR review comments

c4d04c7

See #381

evansiroky merged commit 03e9d2e into auto-deploy May 11, 2021

binh-dam-ibigroup mentioned this pull request Jun 18, 2021

Validation "skipped", view not refreshed after uploading new feed version #395

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

More auto-deploy changes. #381

More auto-deploy changes. #381

Uh oh!

evansiroky commented May 7, 2021 •

edited

Loading

Uh oh!

codecov-commenter commented May 7, 2021 •

edited

Loading

Uh oh!

br648 left a comment

Uh oh!

landonreed May 7, 2021 •

edited

Loading

Uh oh!

evansiroky May 11, 2021

Uh oh!

landonreed May 7, 2021

Uh oh!

evansiroky May 11, 2021

Uh oh!

landonreed left a comment

Uh oh!

binh-dam-ibigroup left a comment

Uh oh!

binh-dam-ibigroup May 7, 2021

Uh oh!

binh-dam-ibigroup May 7, 2021

Uh oh!

evansiroky May 11, 2021

Uh oh!

binh-dam-ibigroup May 7, 2021

Uh oh!

binh-dam-ibigroup commented May 7, 2021

Uh oh!

Uh oh!

More auto-deploy changes. #381

More auto-deploy changes. #381

Uh oh!

Conversation

evansiroky commented May 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Description

Uh oh!

codecov-commenter commented May 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

br648 left a comment

Choose a reason for hiding this comment

Uh oh!

landonreed May 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evansiroky May 11, 2021

Choose a reason for hiding this comment

Uh oh!

landonreed May 7, 2021

Choose a reason for hiding this comment

Uh oh!

evansiroky May 11, 2021

Choose a reason for hiding this comment

Uh oh!

landonreed left a comment

Choose a reason for hiding this comment

Uh oh!

binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

Uh oh!

binh-dam-ibigroup May 7, 2021

Choose a reason for hiding this comment

Uh oh!

binh-dam-ibigroup May 7, 2021

Choose a reason for hiding this comment

Uh oh!

evansiroky May 11, 2021

Choose a reason for hiding this comment

Uh oh!

binh-dam-ibigroup May 7, 2021

Choose a reason for hiding this comment

Uh oh!

binh-dam-ibigroup commented May 7, 2021

Uh oh!

Uh oh!

evansiroky commented May 7, 2021 •

edited

Loading

codecov-commenter commented May 7, 2021 •

edited

Loading

landonreed May 7, 2021 •

edited

Loading