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

feat: gto assign / deprecate / register —auto-push #281

Merged
merged 22 commits into from
Oct 11, 2022
Merged

feat: gto assign / deprecate / register —auto-push #281

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
1c2914f
introduce git_push_tag
Sep 30, 2022
c053fe3
introduce git_push_tag
Sep 30, 2022
a8a71ee
fix echoes suggestion on tag pushing
Sep 30, 2022
78c2651
introduce auto-push option in register command
Sep 30, 2022
f25fb7f
fix expected message in test
Oct 3, 2022
0a3811f
fix test for windows
Oct 3, 2022
46e3061
echo git push tag
Oct 5, 2022
66344b1
add --auto-push option to gto register
Oct 5, 2022
cc379b1
add --auto-push option to gto api unassign
Oct 5, 2022
c7bb538
add --auto-push option to gto api deregister
Oct 5, 2022
5f683fa
add --auto-push option to gto api deprecate
Oct 5, 2022
4ce3170
fix minor bugs
Oct 5, 2022
0d4f768
quote echoed git command hints with "`"
francesco086 Oct 5, 2022
eae7245
throw exception WrongArgs if try to push tag but provided repo does n…
Oct 5, 2022
b13909b
remove prints
Oct 7, 2022
8cad931
remove todo after clarification
Oct 7, 2022
ea2df8b
assign stage to a model that was not registered requires two tags (fi…
Oct 7, 2022
96accc8
fix echo messages in the delete=true case
Oct 7, 2022
17eaa0b
fix use of remote.push (takes a list as argument, not many arguments)
Oct 7, 2022
a2b0b30
mark auto-push feature as experimental
Oct 10, 2022
0f39722
if git push tag fails then raise GTOException, which results in exit …
Oct 10, 2022
3858a3f
cleaned test_git_utils by collecting common code to create mocked repo
Oct 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,5 +138,6 @@ dmypy.json
cython_debug/

.idea
.vscode

gto/_gto_version.py
10 changes: 10 additions & 0 deletions gto/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ def register(
bump_major: bool = False,
bump_minor: bool = False,
bump_patch: bool = False,
auto_push: bool = False,
stdout: bool = False,
author: Optional[str] = None,
author_email: Optional[str] = None,
Expand All @@ -113,6 +114,7 @@ def register(
bump_major=bump_major,
bump_minor=bump_minor,
bump_patch=bump_patch,
auto_push=auto_push,
stdout=stdout,
author=author,
author_email=author_email,
Expand All @@ -129,6 +131,7 @@ def assign(
message: Optional[str] = None,
simple: bool = False,
force: bool = False,
auto_push: bool = False,
skip_registration: bool = False,
stdout: bool = False,
author: Optional[str] = None,
Expand All @@ -144,6 +147,7 @@ def assign(
message=message,
simple=simple,
force=force,
auto_push=auto_push,
skip_registration=skip_registration,
stdout=stdout,
author=author,
Expand All @@ -162,6 +166,7 @@ def unassign(
simple: Optional[bool] = None,
force: bool = False,
delete: bool = False,
auto_push: bool = False,
author: Optional[str] = None,
author_email: Optional[str] = None,
):
Expand All @@ -175,6 +180,7 @@ def unassign(
simple=simple if simple is not None else False,
force=force,
delete=delete,
auto_push=auto_push,
author=author,
author_email=author_email,
)
Expand All @@ -190,6 +196,7 @@ def deregister(
simple: Optional[bool] = None,
force: bool = False,
delete: bool = False,
auto_push: bool = False,
author: Optional[str] = None,
author_email: Optional[str] = None,
):
Expand All @@ -202,6 +209,7 @@ def deregister(
simple=simple if simple is not None else True,
force=force,
delete=delete,
auto_push=auto_push,
author=author,
author_email=author_email,
)
Expand All @@ -215,6 +223,7 @@ def deprecate(
simple: Optional[bool] = None,
force: bool = False,
delete: bool = False,
auto_push: bool = False,
author: Optional[str] = None,
author_email: Optional[str] = None,
):
Expand All @@ -225,6 +234,7 @@ def deprecate(
simple=simple if simple is not None else True,
force=force,
delete=delete,
auto_push=auto_push,
author=author,
author_email=author_email,
)
Expand Down
14 changes: 14 additions & 0 deletions gto/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,12 @@ def callback_sort( # pylint: disable=inconsistent-return-statements
help="Print output in table format",
show_default=True,
)
option_auto_push = Option(
False,
"--auto-push",
is_flag=True,
help="Push created tag automatically (experimental)",
)


@app.callback("gto", invoke_without_command=True, no_args_is_help=True)
Expand Down Expand Up @@ -517,6 +523,7 @@ def register(
bump_patch: bool = Option(
False, "--bump-patch", is_flag=True, help="Bump patch version"
),
auto_push: bool = option_auto_push,
):
"""Create an artifact version to signify an important, published or released iteration

Expand Down Expand Up @@ -544,6 +551,7 @@ def register(
bump_major=bump_major,
bump_minor=bump_minor,
bump_patch=bump_patch,
auto_push=auto_push,
stdout=True,
)

Expand All @@ -562,6 +570,7 @@ def assign(
message: Optional[str] = option_message,
simple: str = option_simple,
force: bool = option_force,
auto_push: bool = option_auto_push,
skip_registration: bool = Option(
False,
"--sr",
Expand Down Expand Up @@ -604,6 +613,7 @@ def assign(
message=message,
simple=simple, # type: ignore
force=force,
auto_push=auto_push,
skip_registration=skip_registration,
stdout=True,
)
Expand All @@ -619,6 +629,7 @@ def deprecate(
simple: str = option_simple,
force: bool = option_force,
delete: bool = option_delete,
auto_push: bool = option_auto_push,
):
"""Deprecate artifact, deregister a version, or unassign a stage

Expand All @@ -642,6 +653,7 @@ def deprecate(
simple=simple, # type: ignore
force=force,
delete=delete,
auto_push=auto_push,
stdout=True,
)
elif version:
Expand All @@ -653,6 +665,7 @@ def deprecate(
simple=simple, # type: ignore
force=force,
delete=delete,
auto_push=auto_push,
stdout=True,
)
else:
Expand All @@ -663,6 +676,7 @@ def deprecate(
simple=simple, # type: ignore
force=force,
delete=delete,
auto_push=auto_push,
stdout=True,
)

Expand Down
29 changes: 29 additions & 0 deletions gto/git_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from git import Repo

from gto.constants import remote_git_repo_regex
from gto.exceptions import GTOException, WrongArgs


def git_clone_remote_repo(f: Callable):
Expand Down Expand Up @@ -67,3 +68,31 @@ def cloned_git_repo(repo: str):
def git_clone(repo: str, dir: str) -> None:
logging.debug("clone %s in directory %s", repo, dir)
Repo.clone_from(url=repo, to_path=dir)

Copy link
Contributor

Choose a reason for hiding this comment

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

Something to consider: it could be that we have some Git tags locally, but don't have them in remote. Then if you run --auto-push in locally cloned repo, you'll decide what to do based on the local repo, but you will try to apply it to the remote one. It could be an error in some cases (you try to remove a remote Git tag that doesn't exist in remote). It could be a incorrect decision (you deprecate a version that doesn't exist in the remote). Looks too complicated to resolve at Friday evening 🍻

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"From great power comes great responsibility" I'd say :D
In the end the auto-push is something you can choose to use or not at every call, as you can choose or not to git push a tag. But if this were not (very) useful, why would you echo a suggestion to push the created tags?

In practice I find hard to imagine to work with a local repo out of sync with the remote. This is the CI/CD practice, isn't it?
In fact I would suggest to be able to set auto-push always true in a config file...

Copy link
Contributor

Choose a reason for hiding this comment

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

In practice I find hard to imagine to work with a local repo out of sync with the remote.

This could happen by chance. But ok, I guess we'll need to accommodate for this in the future. This may happen as part of implementing gto status command, so I'll write it in that ticket.

In fact I would suggest to be able to set auto-push always true in a config file...

Good idea!

Copy link
Contributor

Choose a reason for hiding this comment

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

Example of the currently misleading behavior. Consider you have a git tag [email protected] in remote, but don't have it locally. Then:

$ gto register bug --auto-push
Created git tag '[email protected]' that registers version
Running `git push origin [email protected]`
WARNING:git.remote:Error lines received while fetching: error: failed to push some refs to 'https://github.com/aguschin/example-gto'
Successfully pushed git tag [email protected] on remote.

I think we should fail at push, printing something like: The Git tag [email protected] already exists on remote. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that it would be more user friendly, therefore desirable. But on the other hand, the same exact error would happen if the user does the git push manually, right? So, as the user can choose if to do git push or not, can also choose whether to add the option --auto-push or not.

In conclusion, I don't see much harm in it, but one could make error messages easier to interpret, I agree.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, think I got your point. As I see the difference

  1. We're communicating it incorrectly. We say Successfully pushed git tag.... E.g. the user may assume the CI is triggered in the remote. But the tag was not pushed, and CI was not triggered. So we're confusing user with this message.
  2. Often, a user registers a version or assigns a stage to trigger CI. In this case the goal wasn't achieved, so we should exit with code 1. It could be that user don't want any CI and just want the remote repo to be in this state (so as the Git tag was pushed, we don't need to do anything and it is fine). In this case it may be ok, but I think triggering the CI is more frequent goal here. We may add some tag (--no-fail-if-tags-exist-in-remote e.g.), but I think it's overkill now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, then is it ok if I apply the following change: if git push fails, then echo an appropriate message and exit with code 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

appropriate message being something like "Execution of git push origin [email protected] failed. Make sure your local repository is in sync with the remote."

ok?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, totally. Thanks 🙏🏻

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be it: 0f39722


def git_push_tag(
repo_path: str, tag_name: str, delete: bool = False, remote_name: str = "origin"
) -> None:
repo = Repo(path=repo_path)
remote = repo.remote(name=remote_name)
if not hasattr(remote, "url"):
raise WrongArgs(
f"provided repo_path={repo_path} does not appear to have a remote to push to"
)
logging.debug(
"push %s tag %s from directory %s to remote %s with url %s",
"--delete" if delete else "",
tag_name,
repo_path,
remote_name,
remote.url,
)
remote_push_args = [tag_name]
if delete:
remote_push_args = ["--delete"] + remote_push_args
push_info = remote.push(remote_push_args)
if push_info is not None:
raise GTOException(
msg=f"The command `git push {remote_name} {' '.join(remote_push_args)}` failed. "
f"Make sure your local repository is in sync with the remote."
)
Loading