startersclan / docker-sourceservers

Source / Goldsource dedicated server images built through use of steamcmd. 🐳
Apache License 2.0
47 stars 5 forks source link

Revert workaround for steamcmd +force_install_dir #19

Closed leojonathanoh closed 3 years ago

leojonathanoh commented 3 years ago

Current

Workaround for steamcmd +force_install_dir not yet reverted.

Expectation

Workaround for steamcmd +force_install_dir reverted.

Discussion

As was implemented in #18, it appears that steamcmd has addressed the bug of +force_install_dir not working properly in a new release. See https://github.com/ValveSoftware/steam-for-linux/issues/7843#issuecomment-858100864.

If that is the case, #18 may be reverted.

joeltimothyoh commented 3 years ago

Just wondering about your thoughts concerning use of revert/ as a convention. I personally think every change including reverts are fixes or refactors. @leojonathanoh

leojonathanoh commented 3 years ago

Everything is an implementation, e.g. feature, refactor removal,

But not everything is an addition. e.g. feature, refactor are additions.

revert is a removal. Some use the convention remove, some revert

joeltimothyoh commented 3 years ago

Refactors are changes that only include restructuring of code. Fixes could be either additions or subtractions from code.

Reverts are just exact reversals of any of such code changes. Often it isn’t the case that changes should have to be exactly reversed. If such changes need to be reverted, they’ll simply be another fix specifying reasons for reversion within the commit description and organized under fix/ or docs/ or whatnot. Reversions themselves aren’t a category of change to be organized under; the use of revert/ thus isn’t clearly conventional or clearly adopted in many repos. @leojonathanoh

leojonathanoh commented 3 years ago

Fixes could be either additions or subtractions from code.

Everyone one of those involve additions and subtractions of code. To define a fix as such is too generic.

I think we can agree on these definitions:

Reverts are just exact reversals of any of such code changes. Often it isn’t the case that changes should have to be exactly reversed. Often it isn’t the case that changes should have to be exactly reversed. If such changes need to be reverted, they’ll simply be another fix specifying reasons for reversion within the commit description and organized under fix/ or docs/ or whatnot.

Agreed. So it really depends on what is being reverted. See definitions above.

joeltimothyoh commented 3 years ago

Everyone one of those involve additions and subtractions of code. To define a fix as such is too generic.

Of course it was generic since merely describes what fixes are in response to previous claims on how changes were either additions or subtractions.

I raise this concerning your previous PRa and how reverts were implemented. Reverts are acceptable but I think it’s best not to use them without a custom reason for the revert in the commit messages. And if possible reverts should instead be organized under the most appropriate change category e.g. fix, docs etc. @leojonathanoh

leojonathanoh commented 3 years ago

And if possible reverts should instead be organized under the most appropriate change category e.g. fix, docs etc. @leojonathanoh

True, reverts may a removal and not a fix, in which case it may be classified under revert (i.e. removal).

revert is generally for recent changes (i.e. a short while since the implementation of what is being reverted).

removal is generally for deprecated or unused features.

leojonathanoh commented 3 years ago

in general i prefer the word Remove. According to the definitions above, there's really no need for the term Revert.

joeltimothyoh commented 3 years ago

Often reverts are fixes to what was introduced earlier. The most common category would be fix; docs changes don’t often need to be reverted since but reflections of changes to the application in question. Revert changes such as #20 don’t clearly fall under any of the categories, since #18 was but required due to the bug introduced by the associated game update the change addresses. Perhaps this issue was unique in that workaround #18 had to be introduced. #18 appears more as a hotfix and #20 the reversal of it. Both perhaps are but hotfix changes for the phantomic breaking game update. @leojonathanoh

leojonathanoh commented 3 years ago

If you are speaking in the context of that particular hotfix #18 and that particular revert PR #20, then it might have actually been better to use the term hotfix (workaround) for #18, and the term revert (workaround) / remove (workaround) for #20.

joeltimothyoh commented 3 years ago

If you are speaking in the context of that particular hotfix #18 and that particular revert PR #20, then it might have actually been better to use the term hotfix (workaround) for #18, and the term revert (workaround) / remove (workaround) for #20.

I'd say the revert would be better classified as a hotfix in itself. @leojonathanoh

leojonathanoh commented 3 years ago

so you mean both #18 and #20 should have been a hotfix?

joeltimothyoh commented 3 years ago

so you mean both #18 and #20 should have been a hotfix?

Yes, technically. @leojonathanoh

leojonathanoh commented 3 years ago

updated labels