Gcts update operations #137
Conversation
|
Nice! I really appreciate your refactoring efforts to make the code look nice and follow the Zen of Python. |
|
Can you please "rebase" your branch on the latest master instead of merging master into your PR branch? It makes review much more convenient. |
|
Also, don't be afraid of force pushing your PR branch. For example the "Revert" commits are not useful for review at all. Look at how "git rebase -i ..." works and reorganize and squash your commits please. |
andreihrybo
force-pushed
the
gcts-update-operations
branch
from
a2ea858 to
66a942b
Compare
andreihrybo
force-pushed
the
gcts-update-operations
branch
from
66a942b to
9800ba5
Compare
sap/cli/gcts.py
Outdated
| response = sap.rest.gcts.simple.checkout( | ||
| connection, args.branch, repo=repo, | ||
| no_import=args.no_import, buffer_only=args.buffer_only, | ||
| progress_consumer=LogSugarOperationProgress()) |
There was a problem hiding this comment.
The command line interface which is directly communicating with users should use ConsoleSugarOperationProgress.
sap/cli/gcts.py
Outdated
| } | ||
| if args.sync_clone: | ||
| clone_action = sap.rest.gcts.simple.clone | ||
| parameters['progress_consumer'] = LogSugarOperationProgress() if args.buffer_only or args.no_import else None |
There was a problem hiding this comment.
The command line interface which is directly communicating with users should use ConsoleSugarOperationProgress.
There was a problem hiding this comment.
It seems that TaskOperationProgress also should use console.printout() without _mod_log().info().
There was a problem hiding this comment.
My initial idea was the following:
- have a default simple implementation using logger for "library" usecases
- create a special type for command line interfaces (usint console)
- also be able to have a special type for Ansible which should not print anything but shall collect messages and report them in the json output
Since you have the clear picture of your changes in mind, please adapt it accordingly to the points above.
| abap_modifications_disabled(repo, progress_consumer) if no_import else context_stub(), | ||
| buffer_only_enabled(repo, progress_consumer) if buffer_only else context_stub() | ||
| ): | ||
| repo.clone() |
There was a problem hiding this comment.
The context managers are supposed to temporarily modify repository configuration. In the case of a network error or something like that, the code execution is interrupted and the configuration changes will become persistent. To avoid user confusion, the program should let the end user know how to revert the configuration changes. Look at the example at: https://github.com/jfilak/sapcli/blob/master/sap/cli/gcts.py#L968
There was a problem hiding this comment.
You may need to update the class SugarOperationProgress because you cannot use the construction:
if checkout_progress.recover_message:
console.printerr(checkout_progress.recover_message)
Since the file sap/rest/gcts/simple.py must not use console directly beucase the simple functions can be called from:
- command line interface
- other tools witout standard output
- ansible
sap/rest/gcts/simple.py
Outdated
| return repo | ||
| with ( | ||
| abap_modifications_disabled(repo, progress_consumer) if no_import else context_stub(), | ||
| buffer_only_enabled(repo, progress_consumer) if buffer_only else context_stub() |
There was a problem hiding this comment.
The context managers are supposed to temporarily modify repository configuration. In the case of a network error or something like that, the code execution is interrupted and the configuration changes will become persistent. To avoid user confusion, the program should let the end user know how to revert the configuration changes. Look at the example at: https://github.com/jfilak/sapcli/blob/master/sap/cli/gcts.py#L968
sap/rest/gcts/simple.py
Outdated
| return repo.checkout(branch) | ||
| with ( | ||
| abap_modifications_disabled(repo, progress_consumer) if no_import else context_stub(), | ||
| buffer_only_enabled(repo, progress_consumer) if buffer_only else context_stub() |
There was a problem hiding this comment.
The context managers are supposed to temporarily modify repository configuration. In the case of a network error or something like that, the code execution is interrupted and the configuration changes will become persistent. To avoid user confusion, the program should let the end user know how to revert the configuration changes. Look at the example at: https://github.com/jfilak/sapcli/blob/master/sap/cli/gcts.py#L968
andreihrybo
force-pushed
the
gcts-update-operations
branch
from
19ec2e2 to
9800ba5
Compare
…method to print only status
|
Well done! Thank you. |
2 participants
Add a 'get list' function for gCTS objects and create tests for it.
Add context manager for buffer repository config
Add flags --no-import and --buffer-only to the clone and checkout gCTS operations, and refactor these operations.
If the flags --no-import or were passed to the gcts clone or checkout command, we don’t look at the activity — we just check whether the repository was cloned and whether the branch matches in loop {--wait-for-ready} seconds.
If we use asynchronous cloning with a Task and the --no-import flag, then after the task is completed we only check once that the repository has been cloned by verifying its status, since the task has already been processed during the {--wait-for-ready} seconds.
flag --buffer-only allow us to check activities
VCS_NO_IMPORT - No objects are imported or added to the buffer during an update or clone action.
VCS_BUFFER_ONLY - No objects are imported into the system, but a transport request is generated and added to the buffer. You can manually import the transport request at a later point in time.