search

rycus86 / githooks

Githooks: per-repo and global Git hooks with version control
MIT License
382 stars 20 forks source link

2: Refactor: Offload update to install script #106

Closed gabyx closed 4 years ago

gabyx commented 4 years ago

Continuation on #105

Thanks for quickly reviewing. Tests pass.

coveralls commented 4 years ago

Pull Request Test Coverage Report for Build 678

Changes Missing Coverage Covered Lines Changed/Added Lines %
base-template.sh 37 49 75.51%
cli.sh 43 65 66.15%
install.sh 56 102 54.9%
<!-- Total: 143 223 64.13% -->
Files with Coverage Reduction New Missed Lines %
base-template.sh 1 78.47%
install.sh 4 80.15%
cli.sh 6 82.96%
<!-- Total: 11 -->
Totals Coverage Status
Change from base Build 673: 0.04%
Covered Lines: 2083
Relevant Lines: 2550
💛 - Coveralls
gabyx commented 4 years ago

Tests all pass except arch of course, would be good to go :+1: https://travis-ci.org/github/rycus86/githooks/builds/695247260

gabyx commented 4 years ago

Rebased, thanks for the pace,and your time In that I can make some progress. Otherwise its quite cumbersome, you in australia me in europe. hm...

gabyx commented 4 years ago

Also eveything works: I think I need to look closer to the update from previous version since we have git now I can directly enforce this easily in a a test

gabyx commented 4 years ago

@rycus Could you please open a 'dev' branch and add me to the collaborators such that I can push and merge PRs directly into dev and you can coordinate the update to master which is the release. In this way I can develop a version which consists of several pulls you can review too but dont need to be active all the time.

gabyx commented 4 years ago

Updates from old versions should be fine. I will add a old version update test in another PR. could you merge this. Thanks 🙏

rycus86 commented 4 years ago

Rebased, thanks for the pace,and your time In that I can make some progress. Otherwise its quite cumbersome, you in australia me in europe. hm...

Thanks a lot for your persistence! I try to have a look at this in the morning before my day starts, then I van mostly only have a look at night again (around this time) sadly.

Could you please open a 'dev' branch and add me to the collaborators such that I can push and merge PRs directly into dev and you can coordinate the update to master which is the release. In this way I can develop a version which consists of several pulls you can review too but dont need to be active all the time.

I was thinking about something like this too. We can try it to see how it works. I'm a bit concerned how easy would it be to make review changes though if your branch is 5 PRs ahead of reviews. 🙂 Any suggestions for that case maybe?

rycus86 commented 4 years ago

What's the reason for doing the update (merge) in the install script? I'd have expected that we update the release checkout either daily (from the base template) or from the cli, then we execute the install script of the new commit. Would that work? Trying to understand the reasoning for you choice.

gabyx commented 4 years ago

https://github.com/rycus86/githooks/pull/106#discussion_r436261222 Ehm I forgot this true false for single.install, but we leave it as it falls out anyway in the future

the reasoning to do the merge ind the install script is that its better to do it there because the earlier we merge a new update it can be problematic since we cannot control how the update is gonna proceed from version before to the version merged, think about some legacy patches we apply and other stuff like deprecation warning, if we updated in the basetemplate already its hard to rollback and so on. I rather have everything at the same place for better controll and reasoning. therefore i strongly recommend of having the update procedure at one place. does that make sense. we also would like later to have proper control from where to where we are updating (sha hash) etc

gabyx commented 4 years ago

thx for the review will make the changes later

gabyx commented 4 years ago

the assert_release_clone was thought like: assert that we have a release clone. but never update. if it would be called „update“ than IMO its less clear, does it do fetch and merge too? no it doesnt it just clones... in the case its not there hmm

gabyx commented 4 years ago

Ok summary: We have the following scenarios:

in all cases the dispatch sets --internal-postupdate to tell the (new in case of update) install.sh script to continue after the update if block

gabyx commented 4 years ago

I improved the logic in install.sh:execute_installation should make the steps more clear, my first attempt was corret but convoluted in the last push and integrated the review comments.

gabyx commented 4 years ago

All tests pass -> Good to go :+1:

gabyx commented 4 years ago

Next PR would be: Remove the version number from the 3 files and place it somewhere else in the repo Less merge conflicts and so during developement ...

gabyx commented 4 years ago

FYI: I just tested this and it works:

git checkout master~30
./install.sh
git hooks install --global # trigger update...
## used the following url in the config...
## updateCloneUrl = https://github.com/gabyx/githooks.git
## updateCloneBranch = feature/update-inside-install
##

-> works well :smiley:

gabyx commented 4 years ago

@rycus86 Can you try the following:

cd githooks
./uninstall.sh
rm -rf ~/.githooks
git config --global githooks.autoupdate.updateCloneUrl https://github.com/gabyx/githooks.git
git config --global githooks.autoupdate.updateCloneBranch feature/update-inside-install
# Install old ....
git checkout master~50
./install.sh

Put echo "OLD CLI" and set -x in ~/.githooks/bin/githooks

git hooks install --global

It seems that because the new install.sh overwrites the cli.sh some f**** up behavior happens it executes the cli command install hook a second time!! just random coincidence... I will change the update of the cli, to rm cli.sh and then add a new file this should fix this weird stuff...

gabyx commented 4 years ago

ok good to go.

gabyx commented 4 years ago

Rebased onto master, thanks, lets see if the tests still work. Had to adjust test 094 which gave some merge conflicts...

gabyx commented 4 years ago

https://github.com/rycus86/githooks/pull/106#issuecomment-640106784 -> Last commit reverted since this is not needed anymore.

gabyx commented 4 years ago

Tests fail due to 094, probably made a mistake in the merge and adjustements... will fix it later. If you have time, take a look :-)

gabyx commented 4 years ago

Shall we adjust the indentation for continuation lines in a next PR quickly maybe...

rycus86 commented 4 years ago

Shall we adjust the indentation for continuation lines in a next PR quickly maybe...

Yeah, don't worry about them tok much. Not really sure now how I set them up initially, might have been coincidence/chance with some of those indentations. 🙈

rycus86 commented 4 years ago

Merged, thanks a lot! Have a look at the two questions please, but I think we should be fine.

gabyx commented 4 years ago

Next PR: Remove Version from the 3 scripts -> putting it into VERSION or what woud you like Version.md ?

gabyx commented 4 years ago

Thanks for merging.

rycus86 commented 4 years ago

Next PR: Remove Version from the 3 scripts -> putting it into VERSION or what woud you like Version.md ?

Yeah, VERSION sounds good to me. Alternatively, maybe let's think about dropping a hard-coded version again. I'd like a git hooks version maybe that generates the same version string as it does today, perhaps some formatting with git log, but I can see how these merge conflicts are not helping. So if you think that's something you would rather have a look at, let's try that?