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 support for neuro job list --name #633

Merged
merged 7 commits into from
Apr 3, 2019
Merged

Add support for neuro job list --name #633

merged 7 commits into from
Apr 3, 2019

Conversation

atemate
Copy link
Contributor

@atemate atemate commented Mar 14, 2019

Support neuro ps --name {job-name}

@atemate atemate requested review from asvetlov and shagren March 14, 2019 15:10
@atemate atemate changed the title Add support for neuro job list --name [WIP] Add support for neuro job list --name Mar 14, 2019
python/neuromation/cli/formatters/jobs.py Outdated Show resolved Hide resolved
python/tests/cli/test_formatters.py Outdated Show resolved Hide resolved
python/tests/client/test_jobs.py Outdated Show resolved Hide resolved
python/tests/e2e/test_e2e_jobs.py Outdated Show resolved Hide resolved
python/tests/e2e/test_e2e_jobs.py Outdated Show resolved Hide resolved
python/tests/e2e/test_e2e_jobs.py Outdated Show resolved Hide resolved
@atemate atemate force-pushed the ay/job-name-list branch 2 times, most recently from a2cfde9 to 5916c5e Compare March 18, 2019 10:54
@atemate atemate requested a review from shagren March 18, 2019 11:16
@atemate atemate changed the title [WIP] Add support for neuro job list --name Add support for neuro job list --name Mar 18, 2019
@atemate
Copy link
Contributor Author

atemate commented Mar 20, 2019

Note: maybe the server should return sorted list of jobs with specified name, and the client should choose the last chronologically sorted one. Not doing GET /jobs?name=n&status=failed&status=pending.

@atemate atemate force-pushed the ay/job-name-list branch 3 times, most recently from fe982f1 to 8c496ac Compare March 26, 2019 17:23
@atemate atemate force-pushed the ay/job-name-list branch 2 times, most recently from 3abd598 to b460ba7 Compare March 28, 2019 05:25
@atemate
Copy link
Contributor Author

atemate commented Mar 28, 2019

The Codecov is broken: it does not show exact lines where we have lost coverage.

The coverage decreased because of cli files and client/url_utils.py, which we didn't modify at all :/

python/README.md Outdated Show resolved Hide resolved
@@ -203,6 +204,7 @@ def from_job(
when_humanized = humanize.naturaldate(when_datetime)
return cls(
id=job.id,
name=job.name if job.name else "",
Copy link
Contributor

Choose a reason for hiding this comment

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

name: str is not optional, thus it is never None. should be passed as is, or the type should be changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually, the class of JobDescription has name: Optional[str] = None. When working on the field name I mostly followed the pattern of how the field JobDescription.description is implemented: Optional[str] for JobDescription and str for formatters (TabularJobRow and others).

So left this unchanged.

python/neuromation/cli/formatters/jobs.py Outdated Show resolved Hide resolved
python/neuromation/client/jobs.py Outdated Show resolved Hide resolved
python/tests/e2e/test_e2e_jobs.py Outdated Show resolved Hide resolved
@atemate atemate force-pushed the ay/job-name-list branch from f932f22 to a7dfc39 Compare April 2, 2019 09:58
@atemate atemate requested a review from shagren April 2, 2019 14:49
@atemate atemate force-pushed the ay/job-name-list branch from 4507162 to a5c226e Compare April 2, 2019 15:35
@atemate atemate force-pushed the ay/job-name-list branch from a5c226e to 0b88e2e Compare April 3, 2019 09:13
@neuro-inc neuro-inc deleted a comment from codecov bot Apr 3, 2019
Copy link
Contributor

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

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

Minor: please add change note

@atemate
Copy link
Contributor Author

atemate commented Apr 3, 2019

Minor: please add change note

we update note only on the last PR that implements job names so that no one will start using job-names until they are fully supported

Copy link
Contributor

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

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

Ok. Please add chengenotes to #648 than

@atemate
Copy link
Contributor Author

atemate commented Apr 3, 2019

Ok. Please add chengenotes to #648 than

already 😉

@atemate atemate dismissed shagren’s stale review April 3, 2019 12:52

dismiss because don't see the re-request button

@atemate atemate merged commit ac34672 into master Apr 3, 2019
@asvetlov asvetlov deleted the ay/job-name-list branch April 3, 2019 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants