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

Alternative syntax for lab mr create #127

Closed
jspreddy opened this issue Mar 29, 2018 · 25 comments
Closed

Alternative syntax for lab mr create #127

jspreddy opened this issue Mar 29, 2018 · 25 comments

Comments

@jspreddy
Copy link

jspreddy commented Mar 29, 2018

The lab mr create would be much more helpful if the following options are available.

GitLab MR property options
source_branch -s <source-branch-name>
target_branch -t <target-branch-name>
@jspreddy jspreddy changed the title Additional options in lab mr create. [source, destination, title, assigned, description] Additional options in lab mr create. [source, destination, title, description] Mar 29, 2018
@jspreddy jspreddy changed the title Additional options in lab mr create. [source, destination, title, description] Additional options in lab mr create. [source, destination] Mar 29, 2018
@zaquestion
Copy link
Owner

zaquestion commented Mar 30, 2018

@jspreddy I'm just noticing the description of lab mr create is misleading

Currently only supports MRs into master

This is actually no longer true, as of 0.10 lab supports passing the target branch as a positional argument. Likewise the source branch is based on the currently checked out branch.

Usage:
  lab mr create [remote [branch]]

Do you specifically need them as flags or is this just the result of poor documentation? In either case I'll for sure make sure to improve the docs for lab mr create to detail setting source/target branch and title/description as part of this issue.

@zaquestion zaquestion added this to the 0.10.1 milestone Mar 30, 2018
@jspreddy
Copy link
Author

jspreddy commented Mar 31, 2018

@zaquestion

Yes, I did notice that it supported the target branch.

The issue is that I need to specifically check out the source_branch in order to create a merge request to target_branch.

I want to be able to create merge requests from a branch that is not currently checked out locally.

I want to be able to specify both the source_branch and the target_branch. So that I can create merge requests without having to check out the branch locally.

@jspreddy jspreddy changed the title Additional options in lab mr create. [source, destination] Additional options in lab mr create. [source, target] Mar 31, 2018
@zaquestion
Copy link
Owner

How would this syntax work for you?

lab mr create [remote[/source] [target]]

It seems like this would be the most git-like way to support this functionality, which is the goal.
Ex

lab mr create origin/source_branch target_branch

If that sounds good I can probably put it together this weekend and have it the next release

@zaquestion zaquestion modified the milestones: 0.10.1, 0.11.0 Apr 12, 2018
@zaquestion
Copy link
Owner

Updates to the lab mr create docs have been released in 0.10.1, planning to implement the above in 0.11.0

@jspreddy
Copy link
Author

Sounds good. Can the remote be an optional param?

lab mr create source_branch target_branch

Or do you think that having remote in there is the better way to do this?

@zaquestion
Copy link
Owner

zaquestion commented Apr 15, 2018

Okay after actually going to implement this, I realize that the below, makes no damn sense..

lab mr create [remote[/source] [target]]

The remote doesn't relate to the source branch at all, the only way the above would make sense is if you swapped the source and target. I'd argue thats not very intuitive, in addition its technically a breaking change.

After looking at the git push docs a bit, this seems like appropriate syntax, it also align to the new lab ci trace syntax so thats promising

lab mr create [remote [[source:] <target>]

Ex

lab mr create origin source_branch:target_branch

Ref:

git push --help
       <refspec>...
           Specify what destination ref to update with what source object. The format of a <refspec> parameter is an optional plus +, followed by the source object <src>,
           followed by a colon :, followed by the destination ref <dst>.

The remote kinda has to be there since its the first positional arg and I don't think changing its a good idea. Largely because git enforces the contraints on it's commands

@zaquestion
Copy link
Owner

Additional Thoughts, perhaps lab mr create just isn't well suited for this usecase. lab mr create is largely to make creating a merge request for the current branch as simple as possible. Its very context driven and it seems like the goal here is to have a better general purpose option thats still human friendly.

Implementing the above (lab mr create [remote [[source:] <target>]) should be pretty simple, but I don't think it'll "improve" that command and it doesn't seem like an ideal solution to the perceived usecase here. Particularly since there would still be no way to set the source remote/user.

So, my alternate proposal is a new command to support a new usecase: lab mr new (working title) to support general purpose mr creation. Syntax wise I'm thinking

lab mr new [[<remote>/] <source>] [[<remote>/] <target>]

This makes both remotes settable and optional by default.

lab mr new source_branch target_branch

or explicitly

lab mr new zaquestion/source_branch origin/target_branch

@zaquestion
Copy link
Owner

@jspreddy Any thoughts?

@jspreddy
Copy link
Author

lab mr new zaquestion/source_branch origin/target_branch is this a cross repository merge request?

Is the origin a repository and zaquestion a repository?

@jspreddy
Copy link
Author

I assume that the remotes would have to be the same for both source and target branches. Is that not the case? Can the remotes be different? How does that work?

@zaquestion
Copy link
Owner

zaquestion commented Apr 17, 2018

The remotes can be different, consider the case where you fork a repo and want to create a merge request against the upstream.

In my example, I'm referring to origin as some repo that's been forked by my user and creating an MR from my version to theirs

@jspreddy
Copy link
Author

Ah ok, got it. I was unaware of this feature in gitlab. I thought forking was only available in github. Turns out I was blind while in gitlab. :p

Maybe because I have access to all my orgs' repos and could contribute directly to the repo, I was blind to the existence of fork.

https://docs.gitlab.com/ee/workflow/forking_workflow.html

@zaquestion
Copy link
Owner

@jspreddy any thoughts on which approach to take?

@jspreddy
Copy link
Author

jspreddy commented Apr 17, 2018

lab mr new zaquestion/source_branch origin/target_branch

I like this approach but I am not sure about the new keyword. I think it is confusing especially since we have the create.

lab mr create
lab mr new

I think it might be good to figure out one common format which can accomplish all the requirements.

From what I understand from this thread, I think the requirements are somewhat like this:

Variables and Requirements

Considering the following variables:

  1. remote: which repository is the branch on?
  2. source_branch: the source branch name.
  3. target_branch: The target branch name.

Requirements for the above variables:

  1. Have a default source and target and remotes for each.
    1. source: current <commit_hash> that is checked out in local repo
    2. target: master
    3. remote: origin
  2. Be able to specify the source and target and remotes.
  3. The remotes should be independently specifiable for both the source and target

Scenarios:

Scenario 1:

Consider a scenario where origin/master exists and feature is a local branch that has not been pushed up to origin yet. And it is currently checked out in the local repo in the scope of which the command is run. i.e. the pwd is the git repo.
lab mr new with no other parameters specified should try to create a MR from origin/feature to origin/master. It will fail, but that would be expected behaviour. Had the feature branch been pushed up to origin, the MR would have been created.

Scenario 2:

consider a scenario where origin/develop exists and origin/feature exists. And feature is checked out locally in the pwd git repo.
lab mr new origin/develop should create a MR from origin/<current_commit_hash> to origin/develop.
This will make sure that what we are trying to merge is actually available on origin. Because some one might have forgotten to push up their commit. And by defaulting to the <commit_hash> if the source is not specified, we also handle the scenario of mismatch between local/feature and origin/feature.

Scenario 3:

In the above scenario, if the user specified the source like lab mr new origin/feature origin/develop, then the MR should be created as requested even when the branches are mismatched.

@jspreddy
Copy link
Author

Sorry if this is overwhelming. I am just trying to document the requirements so that we are on the same page. I want to help.

@zaquestion
Copy link
Owner

Couple things of note

  • source_remote is another variable
    • lab mr create currently defaults to the user fork for the source remote, and if set uses the tracked remote e.g. git push --set-upstream origin feature would set feature to track origin
  • lab mr create operates on branches, not shas, it will check if the branch is pushed, but doesn't check for them being in sync.

@zaquestion
Copy link
Owner

zaquestion commented Apr 18, 2018

Thinking more and more that making lab mr create operate as proposed is the best approach, if we can pin down the same level of comfort that the existing command has. We're still pre-1.0 so this is certainly the time to do it. Also I'm inclined to think most users are only using lab mr create with no arguments since the target branch was only requested/added recently.

@bmess You're an opinionated guy 😸 Any thoughts on these proposed changes to lab mr create

Proposed

lab mr create [[<remote>/] <source>] [[<remote>/] <target>]
lab mr create source_remote/source_branch target_remote/target_branch

with the following shorthand

lab mr create target_remote/target_branch
lab mr create // as it works today

Existing

lab mr create [remote [branch]]
lab mr create target_remote target_branch

@zaquestion zaquestion added feedback Needs feedback help wanted needs input and removed feedback Needs feedback labels Apr 23, 2018
@zaquestion zaquestion removed this from the 0.11.0 milestone Apr 23, 2018
@zaquestion
Copy link
Owner

@jspreddy I'm taking this out of the 0.11.0 milestone to unblock the release. I'd like to collect more feedback from people currently using lab, but I largely think moving to the above proposal is the way to go.

@jspreddy
Copy link
Author

Yea, that is good. Don't want to rush things. Expecially since this is going to be a major change in the API.

@zaquestion
Copy link
Owner

Got recently bit by this in #156 -- had an idea.

Change lab mr create to behave by attempting to create an MR with the most recently opened branch.

Use arguments from earlier proposal

lab mr create [[ <remote>] <source>]      [[<remote>] <target>]
lab mr create source_remote/source_branch target_remote/target_branch

This should largely maintain legacy behavior while providing a more extensible API.

@zaquestion zaquestion changed the title Additional options in lab mr create. [source, target] Alternative syntax for lab mr create Aug 27, 2018
@gtrak
Copy link

gtrak commented Sep 23, 2019

+1 on this issue

The use-case I want is to create a standing 'staging->production' MR in my build via CI. Then when I am ready for a prod deploy, I just merge it and wait for the next staging build to recreate it.

In order to do this, I need to:

  • Change the existing syntax to support arbitrary branches origin/staging origin/production works for me.
    I don't think it should rely on working copy, since MRs don't (you would need to push first).
  • Update or delete an existing MR with the same name before recreating.

@zaquestion
Copy link
Owner

@gtrak Did my previous comment with suggested behavior/syntax work for your first bullet? I'm not sure lab should be so clever to support the 2nd bullet within 1 command but that seems doable if you scripted it with a combination of commands. Lets chat a bit about it.

@gtrak
Copy link

gtrak commented Oct 11, 2019

I think your suggested syntax is fine for point 1.

Re 2, that might be too esoteric of a need across users, so I'm happy to script it with multiple commands.

I'm coming from phabricator/arcanist (which I actually dislike for a lot of reasons), but we did this with arc diff --update in CI to update a diff/PR/MR that we'd land manually when we are ready to deploy.

I guess it can be supported in gitlab much easier, by just resetting the source branch of an MR.

@bmeneg
Copy link
Collaborator

bmeneg commented Jun 15, 2021

AFAICT this issue was solved in version 0.22.0 through 63c2db6, which added the --source flag to lab mr create command: now you can pass whatever --source <remote>:<branch> and the target remote/branch is passed through positional args, i.e. lab mr create --source remote:branch target_remote target_branch.

Would that cover the initial case of this issue?

@bmeneg
Copy link
Collaborator

bmeneg commented Feb 8, 2022

This feature has been implemented for quite some time already and since we had no feedback for my last comment I'm going to close it as "solved".

But feel free to reopen it if needed.

@bmeneg bmeneg closed this as completed Feb 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants