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

add yesterday functionality #287

Merged
merged 2 commits into from
Jun 17, 2020
Merged

add yesterday functionality #287

merged 2 commits into from
Jun 17, 2020

Conversation

gfr10598
Copy link
Contributor

@gfr10598 gfr10598 commented Jun 15, 2020

This completes the implementation of processing of yesterday's data at 0600 UTC.


This change is Reviewable

@coveralls
Copy link
Collaborator

coveralls commented Jun 15, 2020

Coverage Status

Coverage increased (+0.5%) to 62.772% when pulling fe12338 on handle-yesterday into 3e6ec5a on master.

@gfr10598 gfr10598 force-pushed the handle-yesterday branch 6 times, most recently from f3596da to 67afc15 Compare June 16, 2020 14:26
Copy link
Contributor

@stephen-soltesz stephen-soltesz left a comment

Choose a reason for hiding this comment

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

:lgtm: one suggestion.

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @gfr10598 and @stephen-soltesz)


job-service/job-service.go, line 86 at r1 (raw file):

	// Recover the date from datastore.
	ctx, cf := context.WithTimeout(context.Background(), 5*time.Second)

nit: I'd recommend passing in the context. It looks like NewJobService is already allocating one context for another function. Putting the call there will make it easier to provide a context from main (or whatever is one level up).

Right now we have a bunch of random background contexts for functions doing complex things at lower levels.

@gfr10598 gfr10598 force-pushed the handle-yesterday branch 2 times, most recently from 504bb23 to d44229b Compare June 17, 2020 02:24
Copy link
Contributor Author

@gfr10598 gfr10598 left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @stephen-soltesz)


job-service/job-service.go, line 86 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

nit: I'd recommend passing in the context. It looks like NewJobService is already allocating one context for another function. Putting the call there will make it easier to provide a context from main (or whatever is one level up).

Right now we have a bunch of random background contexts for functions doing complex things at lower levels.

Added ctx injection here and in other places.

@gfr10598 gfr10598 merged commit 4ce2d00 into master Jun 17, 2020
@gfr10598 gfr10598 deleted the handle-yesterday branch June 17, 2020 02:47
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.

3 participants