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

Protected branch handling -- name of branch #2374

Closed
ciyer opened this issue Sep 28, 2021 · 7 comments · Fixed by #3077
Closed

Protected branch handling -- name of branch #2374

ciyer opened this issue Sep 28, 2021 · 7 comments · Fixed by #3077
Assignees

Comments

@ciyer
Copy link

ciyer commented Sep 28, 2021

Description

PR #1222 introduced support for projects in which the default branch does not allow pushes. To handle this, a new branch is created and the changes are pushed to the new branch. The name of this new branch is a UUID.

Thought the UUID is a robust solution, naming a branch this way makes it difficult to read. As an alternative, I suggest using a timestamp. For example, YYYY-MM-DD_HH-MM_[3 digit random number]. This introduces a potential race condition if multiple branches are created simultaneously, but this is 1. unlikely, and 2. should be solved by generating a new branch name if a push fails.

Having branches names this way makes the branch name easier to interpret.

@Panaetius
Copy link
Member

Why not just date+uuid? And maybe some prefix, like renku-autobranch to clearly differentiate it from user created branches

@lorenzo-cavazzi lorenzo-cavazzi self-assigned this Aug 5, 2022
@lorenzo-cavazzi
Copy link
Member

I suggest using a pattern similar to the one already in place for autosaves.

renku/autobranch/<username>/<protected_branch_name>/<id_or_data>.
For <id_or_data> we could use the short sha of the latest commit.
Not sure adding the username here is relevant, maybe we could leave that out.

Screenshot_20220808_103517

@ciyer
Copy link
Author

ciyer commented Aug 9, 2022

The main goal here is to make it possible to interpret and reconstruct the purpose of these branches, and I think the above suggestions are good ideas to help realize that goal

  • Including autobranch makes the source clear
  • The username explains who is responsible. This is not important for projects with just one dev, but on projects that have multiple contributors, this is helpful.

So, what about the following structure:
renku/autobranch/<username>/<date>/<uuid>
?

@lorenzo-cavazzi
Copy link
Member

I'm fine with dropping the <protected_branch_name> part if you think it's unnecessary; my thought was that a user might not merge the branch right away and having that in the name can help later. That can also be used by the UI to identify the original protected branch and give better feedback when merging.

For the second part, I would argue that using <commit_sha> instead of <date>/<uuid> is a better choice because we won't end up creating multiple branches when is not necessary -- the service will just create the same branch name, try to push and verify the content is the same.

@Panaetius
Copy link
Member

I do think it makes sense to also put in what branch the branch was originally made from, like main or develop or something. So it's clear to the user what branch the auto-branch relates to

@ciyer
Copy link
Author

ciyer commented Aug 9, 2022

Yes, I agree that it makes sense to remember the name of the branch the new branch derives from (I had misunderstood what was meant there before).

Will using the commit hash lead to a unique branch name? E.g., what happens if ask for a project to get upgraded, do not merge that branch, and then change the default session settings. Wouldn't that try to create two branches, both named renku/autobranch/ciyer/main/<hash of main>?

@lorenzo-cavazzi
Copy link
Member

The short_sha refers to the latest commit in the new branch. Unless the change if the very same, the sha will be different.

In your scenario, I expect a new branch to be created.
Still, the naming will prevent the accidental creation of multiple branches (e.g. sending multiple requests from the UI within a short time span, or similar combinations)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants