Deprecation warning added for single installs

gabyx commented 4 years ago

Only a warning, to let user know that single installs are deprecated with further installs.

No changes to any logic. Documentation marks all places as deprecated.

rycus86 commented 4 years ago

Thanks a lot! This looks 👌 I'll wait for Travis then this should be good to go.

coveralls commented 4 years ago

Pull Request Test Coverage Report for Build 629

46 of 48 (95.83%) changed or added relevant lines in 2 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+0.3%) to 83.725%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
cli.sh	14	16	87.5%
<!--	Total:	46	48	95.83%	-->

Files with Coverage Reduction	New Missed Lines	%
base-template.sh	1	75.27%
<!--	Total:	1		-->

Totals
Change from base Build 627:	0.3%
Covered Lines:	2068
Relevant Lines:	2470

💛 - Coveralls

gabyx commented 4 years ago

Jup, I will fix it. Still some errors, from cherrypick merge etc...

gabyx commented 4 years ago

Had some f**** up error in arch tests -> the last thank_you is not executed, I could not find out why. It should have been executed. I took it inside the execute_install function now everything works... Might be a replacement error or something else weird...

execute_installation "$@" || exit 1
echo "A"
echo "B"
echo "C"

only B and C were printed, the A was never printed, also the thank_you which was there was never executed... (? -> weird...)

gabyx commented 4 years ago

Trying to still fix the weird arch tests 103 and 109 (something with permissions and ls -A) ...

gabyx commented 4 years ago

Ok I cannot fix these arch tests 103/109, I dont know also a workaround, locally it works. It seems its travis and its docker setup ? ... See: https://travis-ci.org/github/rycus86/githooks/jobs/688853023#L671

gabyx commented 4 years ago

Can we anyway merge this change?

rycus86 commented 4 years ago

Can you try pulling the latest Arch image, we should be able to reproduce these issues locally, and I believe Travis will always rebuild from the latest image available on Docker Hub, while locally you might use a cached base image.

gabyx commented 4 years ago

Checkd with the latest image. Locally no errors.

gabyx commented 4 years ago

So what shall we do, its a travis problem...

gabyx commented 4 years ago

I would remove the arch test. from travis until the stuff is fixed. we can of course check the arch tes locally until then...

rycus86 commented 4 years ago

Just re-ran the Arch test on master kn Travis and it's failing the same way, so happy to exclude it for now. Is this PR ready from your side?

rycus86 commented 4 years ago

@gabyx left some comments for you, sorry about the delays! It looks mostly OK, just two things I want to highlight:

single installs should now all fail, regardless where/how you're trying to run them
the warning message can just say they're deprecated now and need to clear the setting before trying again, I'd drop the bit about "how this happened" not sure that adds much value to the users

gabyx commented 4 years ago

single installs should now all fail, regardless where/how you're trying to run them I thought the following as already answered above:

--single will become the normal "install only to this repo" command. Up to now there is an additional "... and also make it standalone". So this test will still work and --single is an option which is still useful, Its just not a global install asking for other stuff like "search other repos/install into registered repos..."

Also It can't be remove if we want cd repo && git hooks install. Since that supports this.
I think this is fine? Isn't it?

rycus86 commented 4 years ago

--single will become the normal "install only to this repo" command

Yep, this will be equivalent to the normal install as single installs will eventually just point to the same base template all installations do. Perhaps we could drop the --global flag from that command and just run a non-single install.

Also It can't be remove if we want cd repo && git hooks install. Since that supports this. I think this is fine? Isn't it?

Now that I write this, I'm thinking just failing a single install on running git hooks install is fine, the user gets the error, then we'll make that flag disappear in a (near) future change.

gabyx commented 4 years ago

Ok. Inputs for a near future PR on to this one: The use case for --single is still

cd repo
git hooks install

why should that run a global installation, especially asking for

global shared hooks
asking to search for existing repos
etc I would leave the --single flag for install.sh in. And also dont change the cli.sh, isn't it good as it is?

rycus86 commented 4 years ago

Ah yeah, good point. So git hooks install should still work but shouldn't try to scan for template dirs or offer to set up shared hooks etc. If that's currently done with the --single flag on the install script, I'd change that so that flag can actually just error out with a message that helps the user understand what they need to do. The bit I'm not sure of what happens is when you can git hooks install using an old cli, will that fetch the latest install script and call it with --single? If that's the case then that won't work. If not with this PR then with one in the very near future. So perhaps we just need a nice error message to tell the user what they need to do and let it break. What do you think?

gabyx commented 4 years ago

Exactly: vgit hooks install -> Thats why the whole stuff gets complicated. It fetches the new install.sh and installs an update. Thats why we leave the --single flag on install.sh. It makes perfect sense. If the repo happens to be a "single install" == meaning its standalone (githooks.single.install) as I call it -> we error out with a nice error message in this PR.

Makes that sense? Could we merge in #71 before (are just some small bug fixes)

gabyx commented 4 years ago

Lets test again if the now POSIX compliant tests still fail in ARch

gabyx commented 4 years ago

OK seems good to go. Thanks for merging, then I can go on with more changes... Maybe; Could we merge in #71 before (are just some small bug fixes)

rycus86 commented 4 years ago

I think I get your point now about keeping the --single to keep git hooks install working, but I'm not clear on the implementation here. We're saying that git hooks install should keep working in any case, right? So your changes would effectively only fail updates in repos that are already single installs? What about new single installs, how do we let them know that this is changing soon? Also, perhaps we should have a message (or an error even) for git hooks config single or whatever that's called, so people wouldn't want to change to single repos from existing ones. What do you think?

gabyx commented 4 years ago

Yes thra would make lots of sense . I will add another depr. warning for this case!

Von meinem iPhone gesendet

Am 28.05.2020 um 13:17 schrieb Viktor Adam notifications@github.com:

I think I get your point now about keeping the --single to keep git hooks install working, but I'm not clear on the implementation here. We're saying that git hooks install should keep working in any case, right? So your changes would effectively only fail updates in repos that are already single installs? What about new single installs, how do we let them know that this is changing soon? Also, perhaps we should have a message (or an error even) for git hooks config single or whatever that's called, so people wouldn't want to change to single repos from existing ones. What do you think?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

gabyx commented 4 years ago

Yes your findings are correct. Will only fail for single install repos with this flag...

Von meinem iPhone gesendet

Am 29.05.2020 um 03:06 schrieb Ganriel Nützi gnuetzi@gmail.com:

Yes thra would make lots of sense . I will add another depr. warning for this case!

Von meinem iPhone gesendet

Am 28.05.2020 um 13:17 schrieb Viktor Adam notifications@github.com:

I think I get your point now about keeping the --single to keep git hooks install working, but I'm not clear on the implementation here. We're saying that git hooks install should keep working in any case, right? So your changes would effectively only fail updates in repos that are already single installs? What about new single installs, how do we let them know that this is changing soon? Also, perhaps we should have a message (or an error even) for git hooks config single or whatever that's called, so people wouldn't want to change to single repos from existing ones. What do you think?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

rycus86 commented 4 years ago

Thanks a lot, starting to look good! So to clarify:

new single installs will show the warning but successfully install as single repo
updating single install repos will show a warning and fail
changing single config prints the warning but still works

Did I get these right?

gabyx commented 4 years ago

Thanks a lot, starting to look good! So to clarify:

new single installs will show the warning but successfully install as single repo

yes (hm.. we do this even if its not updatable anymore... but its for the PR's reason, keep it small)

updating single install repos will show a warning and fail

yes

changing single config prints the warning but still works

no -> there should also be a warning, will add this! hm..

Thx for the summary.

gabyx commented 4 years ago

See the cli.sh warning

gabyx commented 4 years ago

Ok, so now also the test for 042 shoudl work.

rycus86 commented 4 years ago

Nice one! I think this is good to go now! Could you please fix up the comment on the .travis.yml and the typo in the message shown to the user? I'll merge it after that.

Thanks a lot for pushing this through!

rycus86 commented 4 years ago

As a side note, why are the Arch tests still failing? I thought changing the ls -A fixed them.

gabyx commented 4 years ago

No it did not I made it Posix compl. but that didnt help. its still a bug ...

Von meinem iPhone gesendet

Am 30.05.2020 um 13:03 schrieb Viktor Adam notifications@github.com:

As a side note, why are the Arch tests still failing? I thought changing the ls -A fixed them.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

rycus86 commented 4 years ago

Awesome, thanks a lot for your work on this PR! 👍

gabyx commented 4 years ago

It will continue, sadly. But now we have a base to work on I think... Thx for mergin. I will refresh the old PR and continue with the deprecation.

Call cli.sh from the release folder
Make an upgrade procedure which is safe and tries the best but doesnt clutter code hopefully. Maybe we can outsource this upgrade into another script such that its fully independent, lets see...

rycus86 commented 4 years ago

Sounds good! Let's just make sure we go with small PRs so it's easy to verify they do what they should. 👌

gabyx commented 4 years ago

Ok :-)

gabyx commented 4 years ago

Just fyi: The problem with small PRs in this case is, that as soon as we really take single install off the place we need to fix all together in a single PR, because its a cobweb, I dont think making multiple PRs will help the problem, so you have to jump from one to the other?, I can however make single commits in one PR which can be digested better. I will see how I start...

rycus86 commented 4 years ago

It all depends on what is in scope of the PR. If it only changes one thing and one thing only, it can stay small.

rycus86 / githooks

Deprecation warning added for single installs #94

Pull Request Test Coverage Report for Build 629

💛 - Coveralls