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

T/73: Improve DX and add new commands #82

Merged
merged 48 commits into from
Jan 11, 2019
Merged

T/73: Improve DX and add new commands #82

merged 48 commits into from
Jan 11, 2019

Conversation

pomek
Copy link
Member

@pomek pomek commented Aug 20, 2018

Suggested merge commit message (convention)

Feature: Introduced a set of new commands which should help developers in daily tasks. Closes #73.

  • New commands:

    • commit - allows committing all changes files that are tracked by Git (a shorthand for mgit exec 'git commit -a')
    • fetch - allows fetching changes in all cloned repositories (a shorthand for mgit exec 'git fetch')
    • pull - allows pulling changes in all cloned repositories and cloning missing ones (it does not check out to specified branch in mgit.json file)
    • push - allows pushing changes in all cloned repositories (a shorthand for mgit exec 'git push')
    • close - allows mering specified branch into current one and removes the merged branch from the local and remote
  • Command update was renamed to sync.

  • Command save-hashes was renamed to save. It accepts two options: --branch or --hash (which is default one). If specified --branch, name of current branches will be saved in mgit.json.

  • Removed command bootstrap. Use sync command instead. Sync command will scan the package directories and compare results with packages saved in configuration file. If there is something that is not defined in mgit.json, it will be printed out.

  • checkout command now allows checking out the project to specified branch: mgit checkout stable will check out all repositories to #stable branch. It can also create a new branch for repositories that contains changes in files tracked by git. Calling mgit checkout -- --branch develop will create the #develop branch in these repositories.

  • Improved help screen for mgit and introduced a help screen for specified command, e.g.: mgit sync --help.

BREAKING CHANGE: Removed the bootstrap command. sync command should be used instead for initializing the repositories.

BREAKING CHANGE: Renamed update command to sync.

BREAKING CHANGE: Renamed save-hashes command to save. It supports two parameters: --branch and --hash which the second one is set as default.

NOTE: mgit checkout branch will check out the repository on #branch. [branch] argument is optional. If it isn't specified, branch name will be taken from mgit.json file.


Additional information

Following PRs should be merged with this PR:

Kamil Piechaczek added 26 commits August 10, 2018 14:46
…hich checks whether changed files could be committed.
@pomek pomek requested a review from Reinmar August 20, 2018 13:37
@pomek
Copy link
Member Author

pomek commented Aug 20, 2018

I didn't change anything in update command because after a few months I still don't understand what and why we wanted to change.

@coveralls
Copy link

coveralls commented Aug 20, 2018

Pull Request Test Coverage Report for Build 259

  • 310 of 317 (97.79%) changed or added relevant lines in 12 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-1.6%) to 97.372%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/commands/checkout.js 21 22 95.45%
lib/commands/commit.js 41 42 97.62%
lib/commands/sync.js 83 88 94.32%
Totals Coverage Status
Change from base Build 256: -1.6%
Covered Lines: 485
Relevant Lines: 492

💛 - Coveralls

@pomek
Copy link
Member Author

pomek commented Sep 13, 2018

One thing that I wanted to add: there is no problem with adding an interactive configuration tool before command execution. There is a huge problem if you want to do it when the command is being executed. If you would like to do that, mgit mustn't use threads. For commands that require some changes at runtime, we need to work on a single thread.

Since this PR introduces new commands, I'm rejecting here every idea that require work on mgit's architecture because it's a wrong place to do that.

@pomek
Copy link
Member Author

pomek commented Sep 13, 2018

Ok, few things have been done:

  • specific options for command must be defined as parameters for mgit (mgit command --params)

  • --message for commit and close command can be specified as a mgit or/and git option (mgit commit -m 'Foo.' or mgit commit -- -m 'Bar.') but the mgit will take a precedence.

    mgit commit -m 'Foo.' -- -m 'Bar.' - only "Foo." will be used

  • Close command remove the local branch and remote automatically. Not sure about that but… I cannot assume that we don't know what we want to do.

  • Improved the help screens and documentation.

@Reinmar
Copy link
Member

Reinmar commented Sep 13, 2018

There is a huge problem if you want to do it when the command is being executed. If you would like to do that, mgit mustn't use threads. For commands that require some changes at runtime, we need to work on a single thread.

I don't agree with this. We can implement commands differently so to still use multiple threads. You assume that when I call mgit close that must execute a single command on all repos. That doesn't not true. Commands can work on a higher level and take care of the general flow so they are able to:

  • execute a subtask on all repos
  • go back to the main thread and e.g. ask the user about something,
  • execute anoter subtask on all repos
  • go back to the main thread

and so on. In fact, we won't avoid that.

Since this PR introduces new commands, I'm rejecting here every idea that require work on mgit's architecture because it's a wrong place to do that.

I understand and I'm completely fine with postponing this. But when you comment that mgit can't work this way because it's not that kind of tool you leave the impression that you want it to work in the current way. And these are two different things – the limitations that we have now and what this tool could do.

@Reinmar
Copy link
Member

Reinmar commented Sep 13, 2018

I reported a separate ticket about mgit close in #84.

@pomek
Copy link
Member Author

pomek commented Sep 13, 2018

At current architecture of mgit it won't work (but I didn't check it) because all shell commands are executed in threads that we don't have access.

And one question - if it somehow will work, how would you like to configure 4 repositories at the same time?

From technical side it could be done something like that:

  1. mgit close
  2. go through all repositories and configure them (use single thread)
  3. again go through all repositories (using configured options, multi-thread)
  4. anything after merge? some configuration? again 2. then 3.
  5. after all, let's finish.

@pomek
Copy link
Member Author

pomek commented Sep 13, 2018

Btw, I wrote a solution a little similar to your post. :D

@ma2ciek
Copy link
Contributor

ma2ciek commented Oct 1, 2018

After updating to the latest commit I'm getting this warning:

Paths to directories listed below are skipped by mgit because they are not defined in "mgit.json":
  - /Users/maciejbukowski/workspace/cloud-features/packages/@ckeditor

Note that in CF due to few reasons we have 3-level deep directories, e.g.:
packages/@ckeditor/ckeditor5-autoformat.

@pomek
Copy link
Member Author

pomek commented Oct 1, 2018

So this option should be a little smarter. If a directory name starts with @, we must scan this directory because it isn't a package but a directory that groups other packages in a single namespace.

@pomek
Copy link
Member Author

pomek commented Oct 2, 2018

@ma2ciek, fixed. Could you test it once again?

@pomek
Copy link
Member Author

pomek commented Oct 30, 2018

@pomek
Copy link
Member Author

pomek commented Nov 15, 2018

3 months after creating the PR, I totally forgot what mgit close does. When it was mgit merge, I could safely use it for merging branches. Now I'm scared that I would break something and I deleted this command from my mind.

@Reinmar
Copy link
Member

Reinmar commented Nov 15, 2018

OK, we can iron this out later on. Let's not worry about this now. I guess we can have two commands. Those who will use mgit close will learn what it does and if not, that should be clarified by the documentation.

@Reinmar

This comment has been minimized.

@pomek

This comment has been minimized.

@Reinmar
Copy link
Member

Reinmar commented Nov 22, 2018

Yes, sorry. I forgot we have another thread for this command. I'll move my feedback there.

@Reinmar
Copy link
Member

Reinmar commented Jan 11, 2019

OK, enough talking. This PR has matured during the last couple of months so we can release it now :D We'll be polishing things on master ;>

@Reinmar Reinmar merged commit 2097c16 into master Jan 11, 2019
@Reinmar Reinmar deleted the t/73 branch January 11, 2019 09:06
Reinmar added a commit to ckeditor/ckeditor5-dev that referenced this pull request Jan 11, 2019
Reinmar added a commit to ckeditor/ckeditor5 that referenced this pull request Jan 11, 2019
Internal: Aligned the repository to the changes in `mgit`. See cksource/mrgit#82.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Redesign the set of mgit commands
5 participants