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

feat: add meaningful data to auto branch names #3077

Merged
merged 13 commits into from Aug 17, 2022

Conversation

lorenzo-cavazzi
Copy link
Member

Description

Change name of branches automatically generated when trying to push to a remote protected branch.
The final branch name will be renku/autobranch/<username>/<protected_branch_name>/<commit_sha>.
The username is the email address to keep it in line with what renku notebooks does with autosaves.

Screenshot_20220812_141331

P.S. I'm not 100% sure that using get_git_user is the best solution. I assume it's fine most of the time, but I can imagine it being potentially wrong when working on the cli. Should I instead pass the local user to push_changes as a parameter? I don't like that solution either and, to be honest, I feel adding that user here is somewhat unnecessary (it's fine for autosaves since they contain personal content, but in general having a username/email in a branch name looks very weird).

This also includes a minor fix for a service error.

Screenshot_20220812_104253

Fixes #2374

@lorenzo-cavazzi lorenzo-cavazzi requested a review from a team as a code owner August 12, 2022 13:01
@lorenzo-cavazzi lorenzo-cavazzi changed the title 2374 protected branches feat: add meaningful data to auto branch names Aug 12, 2022
pushed_branch = uuid4().hex
user = get_git_user(repository)
email = getattr(user, "email", "unknown")
pushed_branch = f"{PRETECTED_BRANCH_PREFIX}/{email}/{old_active_branch}/{last_short_sha}"
Copy link
Member

Choose a reason for hiding this comment

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

I think we should check that the branch name length does not get too long. According to https://stackoverflow.com/questions/60045157/what-is-the-maximum-length-of-a-github-branch-name it's anywhere between 60 and 250 characters. We probably want to shorten old_active_branch down to a minimum of 6 if it's too long or so and if it's still too long, chorten the email?

Copy link
Member Author

Choose a reason for hiding this comment

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

This makes sense. A shortened email might not be very useful after all. Let me check with @ciyer if dropping it would work.
I'll still add the check for the old_active_branch just in case

Copy link

Choose a reason for hiding this comment

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

Then let us drop the email.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I dropped the email and added the logic to limit branch names to 250 chars

renku/core/util/git.py Outdated Show resolved Hide resolved
Panaetius
Panaetius previously approved these changes Aug 16, 2022
@lorenzo-cavazzi lorenzo-cavazzi enabled auto-merge (squash) August 17, 2022 12:07
@lorenzo-cavazzi lorenzo-cavazzi merged commit efc735b into develop Aug 17, 2022
@lorenzo-cavazzi lorenzo-cavazzi deleted the 2374-protected-branches branch August 17, 2022 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Protected branch handling -- name of branch
3 participants