Skip to content

Added job manager class#248

Closed
Onager wants to merge 13 commits into
masterfrom
job_manager
Closed

Added job manager class#248
Onager wants to merge 13 commits into
masterfrom
job_manager

Conversation

@Onager

@Onager Onager commented Sep 10, 2018

Copy link
Copy Markdown
Contributor

This was inspired by some recent conversations, and #247 - this is manager class for handling job registration. We use a similar scheme in Plaso, and it's been a pretty good model for handling different sorts of subcomponent management (registration/deregistration, enabling/disabling).

This should go in after #240 and #226 so we don't complicate things. Putting it out now for discussion in advance.

@Onager Onager requested review from aarontp and berggren September 10, 2018 12:48

@aarontp aarontp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice. I like this pattern, and this makes things a lot cleaner. Just a couple small things. Thanks!

Comment thread turbinia/jobs/manager.py
@@ -0,0 +1,123 @@
# -*- coding: utf-8 -*-

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we might need the copyright blurb here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done, though I'm not sure we actually need the copyright notice in every file. I know this changed at some point in the past for Plaso, for example.

Comment thread turbinia/jobs/manager_test.py Outdated
self.assertIsNotNone(job)
self.assertEqual(job.NAME, 'testjob1')

job = manager.JobsManager.GetJobInstance('testjob1')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Dupe code block?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not sure what intended here, removed.


def testGetJobInstances(self):
"""Tests getting job objects by name."""
job_names = manager.JobsManager.GetJobNames()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Shouldn't we add a job or two first to really test this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That is definitely more useful.

Comment thread turbinia/jobs/sshd.py


class SSHDExtractionJob(TurbiniaJob):
class SSHDExtractionJob(interface.TurbiniaJob):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Don't we need to set a NAME for this Job as well?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yep

@Onager Onager requested a review from aarontp September 20, 2018 12:42
@aarontp

aarontp commented Sep 20, 2018

Copy link
Copy Markdown
Member

Did your changes get pushed? I don't see them in the commit list or the diff.

Onager and others added 10 commits September 21, 2018 11:51
* Docker & Wordpress

* Cleanup

* Fix reviewer comments

* Rename classes & address reviewer comments.

* Fix reviewer comments + add support for gzipped files

* Fix ugly logs

* Fix broken text
* Change wordpress task so tests run deterministically

* Sort list before returning rather than changing data type
* Scaffolding for Jenkins analysis

* Add analysis and tests

* Linter happy

* Allow more input evidence

* Add import of JenkinsAnalysisJob
* Set status and fix linting in jenkins task
* empty shell

* more scaffholding

* make all of this more simple

* Things appear to be working

* typo

* undo some filepath manipulations

* register the new hadoop Job

* cleanup

* renamed

* fix output evidence

* add tests

* typo

* styleguide

* full path to strings

* comments

* sync

* fix tests

* fix py3 tests

* fix py3 harder

* I call this 'bruteforce programming'

* Make _AnalyzeHadoopAppRoot return a list of string, to use the first one as status on the result Evidence

* remove extra tab

* also check at the beggining of the line
@googlebot

Copy link
Copy Markdown

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

@Onager

Onager commented Sep 21, 2018

Copy link
Copy Markdown
Contributor Author

Closing this out, as the git history got screwed up.

@Onager Onager closed this Sep 21, 2018
@Onager Onager mentioned this pull request Sep 21, 2018
@aarontp aarontp deleted the job_manager branch November 11, 2018 02:33
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