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.

Sign up for GitHub

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

Jump to bottom

add Jobs samples #1405

Merged
merged 3 commits into from
Mar 16, 2018
Merged

add Jobs samples #1405

merged 3 commits into from
Mar 16, 2018

Conversation

averikitsch
Copy link
Contributor

No description provided.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 15, 2018
andrewsg
andrewsg requested changes Mar 15, 2018
View reviewed changes
dlp/jobs.py Outdated
type: (Optional) The type of job. Defaults to 'INSPECT'.
Choices:
DLP_JOB_TYPE_UNSPECIFIED
INSPECT_JOB: The job inspected Google Cloud for sensitive data.
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps change this to "The job inspected content for sensitive data"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

dlp/jobs.py Outdated
specified filter in the request.
Args:
project: The Google Cloud project id to use as a parent resource.
filter: (Optional) Filter expressions are made up of one or more
Copy link
Member

Choose a reason for hiding this comment

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

Do we have any further advice to give on what valid filters look like? It's okay if the answer is "no".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

dlp/jobs.py Outdated
parent = dlp.project_path(project)

# If job type is specified, convert job type to number through enums.
from google.cloud.dlp_v2 import enums
Copy link
Member

Choose a reason for hiding this comment

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

I think we can do without this import and just replace enums with google.cloud.dlp.enums where they appear below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

dlp/jobs.py Outdated
# If job type is specified, convert job type to number through enums.
from google.cloud.dlp_v2 import enums
# Job type dictionary
job_type_to_int = {
Copy link
Member

Choose a reason for hiding this comment

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

The keys here (INSPECT, etc.) don't match the keys in the docstring above (INSPECT_JOB, etc) and it looks like they need to be exact matches to work. Let's change either the keys or the docstring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

dlp/jobs.py

# Iterate over results.
for job in response:
print('Job: %s; status: %s' % (job.name, job.JobState.Name(job.state)))
Copy link
Member

Choose a reason for hiding this comment

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

Can we use the .format() method of string replacement for consistency with other samples in this directory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

dlp/jobs.py Outdated
# Iterate over results.
for job in response:
print('Job: %s; status: %s' % (job.name, job.JobState.Name(job.state)))
info_type_stats = job.inspect_details.result.info_type_stats
Copy link
Member

Choose a reason for hiding this comment

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

I think we can do without listing the inspect_details results here; I suspect not all job types (risk analysis etc) will even have that attribute. The Node samples just show the job name, state, and the creation time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

lesv
lesv suggested changes Mar 15, 2018
View reviewed changes
dlp/jobs.py Outdated
DLP_JOB_TYPE_UNSPECIFIED
INSPECT_JOB: The job inspected Google Cloud for sensitive data.
RISK_ANALYSIS_JOB: The job executed a Risk Analysis computation.
Returns:
Copy link
Contributor

Choose a reason for hiding this comment

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

Newline before Returns:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

dlp/jobs.py Outdated
Args:
project: The Google Cloud project id to use as a parent resource.
job_name: The name of the DlpJob resource to be deleted.
Returns:
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

dlp/jobs.py Outdated
'filter.')
list_parser.add_argument(
'project',
help='The Google Cloud project id to use as a parent resource.')
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider striking The Google Cloud

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

dlp/jobs.py Outdated
help='Delete results of a Data Loss Prevention API job.')
delete_parser.add_argument(
'project',
help='The Google Cloud project id to use as a parent resource.')
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

dlp/jobs_test.py Outdated
import pytest
import os

GCLOUD_PROJECT = os.getenv('GCLOUD_PROJECT')
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we prefer GOOGLE_CLOUD_PROJECT for both sides

Copy link
Member

Choose a reason for hiding this comment

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

I agree in principle, but unfortunately the other languages such as Node are still using GCLOUD_PROJECT; perhaps we should standardize across all languages at once rather than try to change it in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jonparrott PTAL
@broady PTAL

We were supposed to standardized on GOOGLE_CLOUD_PROJECT over two years ago.

@lesv
Copy link
Contributor

lesv commented Mar 16, 2018

@averikitsch Can you merge, or do you need Andrew or I to do it?

@lesv
Copy link
Contributor

lesv commented Mar 16, 2018

@andrewsg @jonparrott This repo should really have kokoro or something checking on submit.

@theacodes
Copy link
Contributor

It has travis. No idea why it's not running.

@averikitsch averikitsch merged commit 90a1166 into GoogleCloudPlatform:dlp Mar 16, 2018
@andrewsg
Copy link
Member

andrewsg commented Mar 17, 2018 via email

@theacodes
Copy link
Contributor

theacodes commented Mar 17, 2018 via email

@averikitsch averikitsch deleted the dlp-jobs branch July 16, 2018 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewsg andrewsg andrewsg approved these changes

@lesv lesv lesv approved these changes

Assignees

@andrewsg andrewsg

@lesv lesv

Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@averikitsch @lesv @theacodes @andrewsg @googlebot