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 fds push and save #34

Merged
merged 14 commits into from
Jun 14, 2021
Merged

Add fds push and save #34

merged 14 commits into from
Jun 14, 2021

Conversation

deanp70
Copy link
Member

@deanp70 deanp70 commented May 25, 2021

This adds the functionality to fds push to both git and dvc remotes at the same time, as well as fds save which efficiently adds, commits, and pushes all changes in the project to a given remote.

@deanp70 deanp70 requested a review from guysmoilov May 25, 2021 13:28
And added the improved error handling to `push` and `save`
# Conflicts:
#	fds/services/fds_service.py
@guysmoilov guysmoilov added the enhancement New feature or request label May 25, 2021
fds/cli.py Show resolved Hide resolved
help='saves all project files to a new version and pushes them to your remote'
)
parser_save.add_argument('-gr', '--git-remote', help="git remote name, default 'origin'", default="origin")
parser_save.add_argument('-dr', '--dvc-remote', help="dvc remote name, default 'origin'", default="origin")
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a flag for the branch name?

Copy link
Member Author

Choose a reason for hiding this comment

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

I want it to be simple. It's probably also a bad idea to have these flags. I'm actually thinking of removing them, once we implement the "check if there is a remote added for both, then just run it" it has to be dead simple

Copy link
Member

Choose a reason for hiding this comment

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

The branch flag is a nice-to-have since 90% of the time it will be the same branch you're currently on.
But I strongly disagree about removing these flags - I think you'll make the tool unusable in many cases, and that whoever doesn't want to change the remote names, doesn't have to

Copy link
Member

Choose a reason for hiding this comment

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

One thing which WOULD be nice is to automatically detect the default remote based on .dvc/config and use that as the default value, but that's more advanced - we're working on that ability in #53

fds/services/dvc_service.py Outdated Show resolved Hide resolved
fds/services/fds_service.py Outdated Show resolved Hide resolved
fds/services/fds_service.py Outdated Show resolved Hide resolved
fds/services/fds_service.py Outdated Show resolved Hide resolved
fds/services/git_service.py Outdated Show resolved Hide resolved
fds/services/git_service.py Outdated Show resolved Hide resolved
@deanp70 deanp70 requested a review from guysmoilov May 28, 2021 08:46
@deanp70
Copy link
Member Author

deanp70 commented Jun 14, 2021

I fixed some of the changes you made. The tests for DVC all fail, not sure if it's related to what I'm doing.

help='saves all project files to a new version and pushes them to your remote'
)
parser_save.add_argument('-gr', '--git-remote', help="git remote name, default 'origin'", default="origin")
parser_save.add_argument('-dr', '--dvc-remote', help="dvc remote name, default 'origin'", default="origin")
Copy link
Member

Choose a reason for hiding this comment

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

The branch flag is a nice-to-have since 90% of the time it will be the same branch you're currently on.
But I strongly disagree about removing these flags - I think you'll make the tool unusable in many cases, and that whoever doesn't want to change the remote names, doesn't have to

help='saves all project files to a new version and pushes them to your remote'
)
parser_save.add_argument('-gr', '--git-remote', help="git remote name, default 'origin'", default="origin")
parser_save.add_argument('-dr', '--dvc-remote', help="dvc remote name, default 'origin'", default="origin")
Copy link
Member

Choose a reason for hiding this comment

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

One thing which WOULD be nice is to automatically detect the default remote based on .dvc/config and use that as the default value, but that's more advanced - we're working on that ability in #53

fds/services/fds_service.py Outdated Show resolved Hide resolved
fds/services/fds_service.py Outdated Show resolved Hide resolved
@guysmoilov guysmoilov merged commit 2eaa04a into main Jun 14, 2021
@guysmoilov guysmoilov deleted the add-fds-push-and-save branch June 14, 2021 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants