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

Auto deploy feed version #361

Merged
merged 76 commits into from
Jun 15, 2021
Merged

Auto deploy feed version #361

merged 76 commits into from
Jun 15, 2021

Conversation

br648
Copy link
Contributor

@br648 br648 commented Jan 18, 2021

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

Auto deploy logic updated to match: https://docs.google.com/document/d/1-ktTBm2phT_7rIVQgk5XbyxT7VtjQnpll4hIyDP8AhQ/edit#heading=h.5s90wltgha1q

Copy link
Contributor

@landonreed landonreed left a comment

Choose a reason for hiding this comment

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

A few minor things. I still need to review the tests a bit more. Also, we need to get the GH actions PR (#360) reviewed and merged into dev before this one now that Travis CI is turned off and we're not getting CI pass/fail indications.

@br648 br648 requested a review from landonreed January 20, 2021 17:15
@br648 br648 removed their assignment Jan 20, 2021
@codecov-io
Copy link

codecov-io commented Jan 21, 2021

Codecov Report

Merging #361 (238d4c6) into dev (11a5597) will increase coverage by 1.86%.
The diff coverage is 59.18%.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev     #361      +/-   ##
============================================
+ Coverage     26.61%   28.48%   +1.86%     
- Complexity      655      723      +68     
============================================
  Files           179      180       +1     
  Lines          9227     9356     +129     
  Branches       1249     1284      +35     
============================================
+ Hits           2456     2665     +209     
+ Misses         6533     6405     -128     
- Partials        238      286      +48     
Flag Coverage Δ Complexity Δ
unit_tests 28.48% <59.18%> (+1.86%) 0.00 <27.00> (ø)

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

Impacted Files Coverage Δ Complexity Δ
...om/conveyal/datatools/editor/datastore/FeedTx.java 0.00% <ø> (ø) 0.00 <0.00> (ø)
...ools/manager/controllers/api/StatusController.java 33.92% <ø> (+12.50%) 9.00 <0.00> (+4.00)
...com/conveyal/datatools/manager/jobs/DeployJob.java 24.11% <0.00%> (+5.79%) 33.00 <0.00> (+9.00)
...conveyal/datatools/manager/jobs/MergeFeedsJob.java 78.94% <0.00%> (-0.19%) 120.00 <0.00> (ø)
...l/datatools/manager/jobs/ProcessSingleFeedJob.java 84.28% <0.00%> (+0.22%) 20.00 <0.00> (+1.00)
...com/conveyal/datatools/manager/models/Project.java 66.66% <ø> (ø) 6.00 <0.00> (ø)
.../manager/controllers/api/DeploymentController.java 11.57% <35.00%> (+2.52%) 3.00 <1.00> (+1.00)
.../conveyal/datatools/manager/models/Deployment.java 15.54% <47.05%> (+11.45%) 19.00 <5.00> (+15.00)
...conveyal/datatools/manager/jobs/AutoDeployJob.java 64.38% <64.38%> (ø) 12.00 <12.00> (?)
...nveyal/datatools/common/status/MonitorableJob.java 82.11% <75.00%> (-0.54%) 12.00 <0.00> (ø)
... and 19 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 11a5597...238d4c6. Read the comment docs.

@landonreed landonreed assigned br648 and unassigned landonreed Jan 21, 2021
@landonreed
Copy link
Contributor

@br648, I just made some changes to provide access to MonitorableJob#subJobs and updated the auto deploy test class to make use of that (though the assertions have not been updated yet). Once you're able to update the assertions, I think we can assign to @binh-dam-ibigroup for review.

@landonreed
Copy link
Contributor

OK, I think this is ready for @binh-dam-ibigroup. @br648, feel free to take another look at my latest tweaks as well.

@landonreed landonreed removed their assignment Jan 21, 2021
Consolidate AutoDeployJob initialization in ProcessSingleFeedJob
@br648 br648 assigned evansiroky and unassigned br648 Apr 30, 2021
@br648
Copy link
Contributor Author

br648 commented Apr 30, 2021

@evansiroky, thanks for the update. I have now merged this into the auto deploy branch.

Copy link
Contributor

@evansiroky evansiroky left a comment

Choose a reason for hiding this comment

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

I think this is functionally complete. We still need to make a UI that will go with these changes, so let's not merge to dev just yet, or if we do, we'll want to make sure we somewhat thoroughly try out this AutoDeploy stuff out before deploying to a production environment.

@evansiroky evansiroky assigned br648 and unassigned evansiroky May 2, 2021
Comment on lines 576 to 581
if (!status.error) {
// A new FetchSingleFeedJob could be started after the AutoDeployJob has made sure no fetching jobs exist and
// the DeployJob has started. Therefore this new feed version wouldn't result in a new DeployJob getting kicked
// off since there was already one running. To catch this new feed version another auto deploy job is started.
DataManager.heavyExecutor.execute(new AutoDeployJob(deployment.parentProject(), owner));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Something that just came to mind is that this could result in an endless loop of deployments. So if a deployment is initialized, then the deploy job finishes successfully there would be no stopping point within either this job or the AutoDeploy job until an error occurs.

@binh-dam-ibigroup binh-dam-ibigroup mentioned this pull request May 7, 2021
5 tasks
Copy link
Contributor

@binh-dam-ibigroup binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

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

LGTM, just one minor formatting suggestion.

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.

6 participants