search

rycus86 / githooks

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

Bugfix: Always setting the internal update flag is wanted #143

Closed gabyx closed 3 years ago

gabyx commented 3 years ago

This is needed for the legacy check [ -z "$INTERNAL_UPDATED_FROM_COMMIT" ] in legacy_transform_after_update, which should only opt-in from old installs.

We forgot to make --internal-update-from bomb proof: This should hopefully fix this.

gabyx commented 3 years ago

Thanks for mergin. Just wittnessed this bug, since no update happend when I installed. And legacy trasnform kicked in because of the buggy test in legacy_transform_after_update which I had no other solution so far. But with this fix. Since we now always have the --internal-updated-from flag set we have good update behavior from now on... 🤞

coveralls commented 3 years ago

Pull Request Test Coverage Report for Build 862

Totals Coverage Status
Change from base Build 860: 0.01%
Covered Lines: 2370
Relevant Lines: 2946
💛 - Coveralls
rycus86 commented 3 years ago

This is why I was suggesting perhaps we should ditch commit tracking and just run transformations based on current local state. 😉

gabyx commented 3 years ago

and I was argueing that current local state is basically the commit sha ;) as with the go rewrite I make good progress. almost done for base templ. but I really see now why this is bash thing even though its easy and understandable its so errornous where with go its 500% more maintainable and less nerve wracking when thinking about what can happen even with global state etc ... we have sooo much funcs we share ;)

Von meinem iPhone gesendet

Am 15.10.2020 um 21:04 schrieb Viktor Adam notifications@github.com:

Merged #143 into master.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

gabyx commented 3 years ago

The underlying problem is more that we did not correctly implemented code for being sure in all cases we can distinguish new from old code, this is especially the case from when we needed this feature in PR125. where we didnt thorougly thought it through. maybe we should think about the several stages of the install like for database transactions and how it interacts with versioning ... dunno

Von meinem iPhone gesendet

Am 15.10.2020 um 21:04 schrieb Viktor Adam notifications@github.com:

Merged #143 into master.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

rycus86 commented 3 years ago

Which is my point exactly. 🙂 I don't think we should have databases for upgrade tracking and such, just keep everything simple - even if that means maybe being slightly slower on upgrades (if you need more checks to determine what needs to be upgraded).

gabyx commented 3 years ago

Lol sorry for my wrong wording: I rather mean supporting older versions (versioning) in any software is always hard and alteady led me into headaches. The install ans update thing in this code base is kind of nice, still a bit convolutet be cause of several entry points into install.sh etc... and thats what all the versioning makes hard. I agree keep it as simple as possible but not sacrificing clean code by introducing to much legacy state which is only there to keep updates right in the future. As less versioning state as possible for my taste. Efficiency ah not a problem. it needs to be safe. I thought (transaction like) what are the steps for an update etc maybe we need clearer structuring, because now I also have troubles understanding the install, whats the difference to an update (currently not much, so install.sh is basically always a graceful update), what are the granulat steps/transactions during an update

Von meinem iPhone gesendet

Am 16.10.2020 um 07:48 schrieb Viktor Adam notifications@github.com:

Which is my point exactly. 🙂 I don't think we should have databases for upgrade tracking and such, just keep everything simple - even if that means maybe being slightly slower on upgrades (if you need more checks to determine what needs to be upgraded).

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

rycus86 commented 3 years ago

Yeah, I see your point. I think we're probably on opposite sides on what we think is simple vs easy to maintain I guess. But don't get me wrong, you're implementation is good, I just wish we didn't need to deal with this. 🤷‍♂️

gabyx commented 3 years ago

I wished too :).

Von meinem iPhone gesendet

Am 16.10.2020 um 08:42 schrieb Viktor Adam notifications@github.com:

Yeah, I see your point. I think we're probably on opposite sides on what we think is simple vs easy to maintain I guess. But don't get me wrong, you're implementation is good, I just wish we didn't need to deal with this. 🤷‍♂️

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.