rycus86 / githooks

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

Make single installs deprecated. #93

Closed gabyx closed 4 years ago

gabyx commented 4 years ago

This is PR Nr.1 towards making single installs completely disappear. In this PR only:

gabyx commented 4 years ago

Once this is reviewed (maybe also merged, i dont know), I continue to incorporate in another PR the changes needed to symlink the base-template.sh. So we need to wait till all PR's Nr1 till the end are reviewed. And merge them together into the master. @rycus86 : thanks for the review. In the next PR I would like to incorporate also renaming and migration changes to config variable names and the registration file. e.g.

Why?, Because we anyway need a small migration function which needs to work, so we can just include these changes there as well. What do you think?

coveralls commented 4 years ago

Pull Request Test Coverage Report for Build 649


Changes Missing Coverage Covered Lines Changed/Added Lines %
base-template.sh 33 45 73.33%
cli.sh 17 29 58.62%
install.sh 74 117 63.25%
<!-- Total: 131 198 66.16% -->
Files with Coverage Reduction New Missed Lines %
base-template.sh 3 78.31%
uninstall.sh 7 81.66%
install.sh 60 68.93%
cli.sh 61 82.61%
<!-- Total: 131 -->
Totals Coverage Status
Change from base Build 643: -5.4%
Covered Lines: 1923
Relevant Lines: 2454

💛 - Coveralls
gabyx commented 4 years ago

I dont know why 2 tests in test-arch fail. Locally they dont. Its the same shitty problem...?

rycus86 commented 4 years ago

I'm a bit confused with this PR again, from the title I thought this is going to be the first step: deprecating single installs as best as we can, failing new single installs, then that should be merged.

This PR also seems to change updates, versioning, and a few.other things as well. Can we please have one change per one PR for now? That might also help narrowing down the problem with the Arch tests as well.

gabyx commented 4 years ago

The problem is, these changes we make in your envisioned PR will all be reverted and refactored again after we have another PR, For my point of view this is very cumbersome and tedious. Look at the two commits. The last one is only docker related (thats why 26 files...). I will try to norrow down the arch problems later this day maybe. With this PR its a tradeoff between changes and the next changes we are going to do -> wrapping the base-template. Please have a look.

gabyx commented 4 years ago

This changes should be mergable yes.

rycus86 commented 4 years ago

The problem is, these changes we make in your envisioned PR will all be reverted and refactored again after we have another PR

Yes, I know, but I wanted to give people a chance to update to the version with the warning before removing support altogether. I'm happy to work on a PR that does only that, but it will be a couple of days at least before I'll have time to look into that most likely.

gabyx commented 4 years ago

Aha, ok. Sorry there was a missunderstanding, then. I will try to incorporate only such a change. Do you want a PR which shows a warning, and the single install feature still continues to work, but telling the user that he/she should switch single off...?

rycus86 commented 4 years ago

That's OK, sorry if I didn't explain myself properly. I think what we could perhaps do is, print a warning and fail the installs with the single flag, and also warn and fail updating single install repos. I think that's all we can do right? We don't change single install repos in any other cases, so this is probably all we can do to warn users.