Bug #6657
closed
[SDKs] arv-copy has an unhelpful error message when `git` command is not available
100%
Description
When arv-copy is asked to copy a pipeline template recursively to a --dst-git-repo, it requires the `git` command in order to run. When the `git` command is not available, my expectation would be that I might get an error something like:
- arv-copy --src qr1hi --dst 7lnae --project-uuid 7lnae-j7d0g-9r429yt0rcrdl8x --dst-git-repo jr17/test qr1hi-p5p6p-8584ylktoumirn0
Error: git command is not available, please ensure it is available in the PATH
What actually happens is the following stack trace:
- arv-copy --src qr1hi --dst 7lnae --project-uuid 7lnae-j7d0g-9r429yt0rcrdl8x --dst-git-repo jr17/test qr1hi-p5p6p-8584ylktoumirn0
Traceback (most recent call last):
File "/usr/local/bin/arv-copy", line 4, in <module>
main()
File "/usr/local/lib/python2.7/dist-packages/arvados/commands/arv_copy.py", line 128, in main
src_arv, dst_arv, args)
File "/usr/local/lib/python2.7/dist-packages/arvados/commands/arv_copy.py", line 272, in copy_pipeline_template
copy_git_repos(pt, src, dst, args.dst_git_repo, args)
File "/usr/local/lib/python2.7/dist-packages/arvados/commands/arv_copy.py", line 371, in copy_git_repos
migrate_jobspec(component, src, dst, dst_repo, args)
File "/usr/local/lib/python2.7/dist-packages/arvados/commands/arv_copy.py", line 344, in migrate_jobspec
copy_git_repo(repo, src, dst, dst_repo, script_version, args)
File "/usr/local/lib/python2.7/dist-packages/arvados/commands/arv_copy.py", line 617, in copy_git_repo
cwd=os.path.dirname(local_repo_dir[src_git_repo]))
File "/usr/local/lib/python2.7/dist-packages/arvados/util.py", line 45, in run_command
p = subprocess.Popen(execargs, **kwargs)
File "/usr/lib/python2.7/subprocess.py", line 679, in init
errread, errwrite)
File "/usr/lib/python2.7/subprocess.py", line 1249, in _execute_child
raise child_exception
OSError: [Errno 2] No such file or directory
Updated by Brett Smith over 9 years ago
- Category set to SDKs
- Target version set to 2015-08-05 sprint
- Story points set to 0.5
We should implement this exactly as specified. arv-copy should check for git up front so it can report this error right away any time it is asked to perform an operation that requires copying git repositories, before it copies anything.
Updated by Radhika Chippada over 9 years ago
- Assigned To changed from Peter Amstutz to Radhika Chippada
Updated by Radhika Chippada over 9 years ago
3282d76febc482aab302a44bf594bcf9591ff868
arv-copy command and documentation are updated.
Updated by Radhika Chippada over 9 years ago
- Subject changed from arv-copy has an unhelpful error message when `git` command is not available to [SDKs] arv-copy has an unhelpful error message when `git` command is not available
- Status changed from New to In Progress
Updated by Peter Amstutz over 9 years ago
- The check for git needs to happen in
copy_pipeline_instance()before it does anything else. It should go before the checkif not args.dst_git_repo: - Instead of raising an exception, it should use
abort - In the documentation, we should avoid advising the user to install a package using
apt-getbecause they may be using a different platform, such as Red Hat. I think saying "you must have git installed" is sufficient.
Updated by Brett Smith over 9 years ago
Peter Amstutz wrote:
- The check for git needs to happen in
copy_pipeline_instance()before it does anything else. It should go before the checkif not args.dst_git_repo:
Don't we also need it in copy_pipeline_template? Like I said in note-1, this check should be done before any copy operation that requires access to a git repository.
Updated by Radhika Chippada over 9 years ago
Updated both copy_pipeline_instance and copy_pipeline_template methods.
Updated by Radhika Chippada over 9 years ago
Peter said:
The check for git needs to happen in copy_pipeline_instance
I implemented this suggestion (updated copy_pipeline_instance and copy_pipeline_template methods). However, I am not sure if this is the right place to check for git version? It appears that there are other places where the copy_git_repo is invoked (for example: migrate_jobspec). Should I move this check back into the copy_git_repo method?
Thanks.
Updated by Brett Smith over 9 years ago
Radhika Chippada wrote:
Peter said:
The check for git needs to happen in copy_pipeline_instance
I implemented this suggestion (updated copy_pipeline_instance and copy_pipeline_template methods). However, I am not sure if this is the right place to check for git version? It appears that there are other places where the copy_git_repo is invoked (for example: migrate_jobspec). Should I move this check back into the copy_git_repo method?
I don't think that will meet the second half of the functional requirements in note-1. If you do that, arv-copy may copy other objects the pipeline uses before it tries to copy the git commits and fails. This will waste the user's time and clutter their project, which is undesirable.
migrate_jobspec only gets called from copy_git_repos, which only gets called from copy_pipeline_instance and copy_pipeline_template. So checking at the top of the pipeline functions should be sufficient.
Updated by Radhika Chippada over 9 years ago
Brett said:
migrate_jobspec only gets called from copy_git_repos, which only gets called from copy_pipeline_instance and copy_pipeline_template. So checking at the top of the pipeline functions should be sufficient
Yes, just checked that it is only executed in this context. So, we should be all set. Thanks.
Updated by Radhika Chippada over 9 years ago
- Status changed from In Progress to Resolved
- % Done changed from 0 to 100
Applied in changeset arvados|commit:ccf30f40f46ea450d7ab3766f0923b486a7450d3.