This issue was moved to a discussion.
You can continue the conversation there. Go to discussion →
[Suggestion] Add Changelog? #155
Comments
|
Definitely a good idea although I think CHANGELOG.md would be more user-friendly it would also be more annoying to maintain so I'm open to either proposed method. |
|
I support this idea, as long as we dont require it for PRs. The more problematic part is removing stuff which got fixed upstream by magento |
|
Agreed, it shouln't be a requirement . I'd prefer the "easier to maintain" solution. Add a first label for kind of this PR (bug/patch/...). I also wouldn't remove entries that got fixed in later Magento version. It should be enough to add a second "branch-label" that match magentos versions, where the issue was fixed. |
|
Don't know where to ask ... ... is there any "schedule" for merging approved PRs? I don't want to stress someone, but there are some PRs that should go live ASAP .... (i know i could use a/my fork, but i definitly want to rely on "official" LTS) |
|
+1 for making human readable changelog. Git history is hard to read because we use non linear history (merge commits) and commit descriptions/titles are also not too nice. List of merged PR's is also not a solution (in the form it is right now) as you don't know which PR was applied on the current branch. I think we should require every PR to add appropriate changelog entry. |
|
But I would like to see references to Issues in commit messages |
|
@Flyingmana can you please share the command you will use to generate a changelog based on the git log? |
|
I "slightly" improved |
|
+1 for the one who added some labels ;) |
|
See below for example changelog generated form git history:
In my opinion we should either improve pull requests:
Or maintain the changelog manually.
|
|
I've just merged a pull request in a slightly different way then before (204cb68) How do you like it? |
|
So if we are going to use this method to maintain a changelog going forward should we just always squash PRs when merging and clean up the commit message to be descriptive but succinct at the time of merge? I've not tried this but it seems it could be easy and still yield good results. |
|
I just tested the "Squash and Merge" method on #242 and here is what the result looks like on the commit history with yesterday's merge for a comparison. It automatically added the PR # to the commit message which wasn't there before and you can modify the message and the longer description. Seems pretty solid to me but this is just one single-commit PR so I don't know what other issues there could be. It does seem to maintain the original committer "credit" though which is definitely important. |
|
Ignoring all other discussion in this thread, I'm 100% against squashed merges. I think if we or the contributor prefer less commits in the log, then that should be done prior to issuing the PR. |
|
maybe there should be contributing guidelines. only one commit by squashing before the PR. the commit should describe what has changed/improved/fixed and why. a signature line - who is the author of the commit (name and email) and maybe a line with tested magento version (for later reference) and maybe a list of people having approved the commit. |
|
Dislike for squashed commits ... (cant see that selected branch is already merged)
How about ...
Should be enough to keep a CHANGELOG.md manually up2date. |
|
I'm not seeing that requiring squashing before PR is any better than "Squash and Merge".. If the submitter provides a single clean commit with a good commit message then there is no reason to use Squash and Merge, but for multi-commit PRs or PRs with not-great commit messages (and then there is the chicken-and-egg scenario making it hard to add the PR # to the commit message) the end result is the same with less hassle. During development I think it is useful to see multiple commits as requested changes are incrementally committed because it makes approval process more clear. Requiring that submitters squash and force-push between each update is just going to result in a lot of "Looks good but please squash your commits" messages and unnecessary delays when there is a "Squash and Merge" button which does almost the same thing. If the goals are:
then from what I've seen "Squash and Merge" is a useful tool which meets all of the requirements in cases where the contributor provides a poor commit messages or multiple-commit history. @sreichel That is only an issue for the submitter.. However, is there not a button on the PR page for you that says something like "Delete branch"? @LeeSaferite Can you explain why you're 100% against "Squash and Merge"? All of that said, I'm fine with adopting whatever strategy as long as it isn't like Magento 2's method which is apparently to rewrite contributors commits fresh with no mention of the original contribution... (does this look familiar?) |
|
Alternative suggestion. |
|
@colinmollenhour of course I have a "Delete Branch" button, but ... just have to delete local branches too in time, to keep overview whats already merged. @Flyingmana "Alternative solutions" .... https://gist.github.com/sreichel/215a05b524ff2201e896e3273922d6bf? Took some initial time, but maintaining it should be manageable. (???) |
|
I agree with @sreichel.
Like this, we have all information needed to do a Changelog. |
|
Agreeing with a lot of the points made here. 👍 for sqashing all PRs, makes cherry-picking commits much easier when backporting, etc. I think given this repo's context/usage the commit message's header should, if possible, describe the problem instead of what changed. Makes the history much easier to digest IMO, specially for potentially new users. We are currently considering this project for one of our sites, sadly after reviewing the current git history of the 1.9.3.x branch I didn't feel very confident, to say the least. I realize the maintainers are unpaid and free time is precious, but git discipline should be sorted out as soon as possible, the longer this ticket slumbers the bigger the mess is gonna get. Personally I'm not a big fan of the current branching model, having huge import/merge commits between version bumps is error prone, but then again there probably won't be many upstream Magento 1.x releases in the future. To verify everything is in order I did some house cleaning:
After that I did a directory compare for the
Not sure if I missed some commits for the last two bullet points, maybe someone else can shed some light on this. Pushed the cleaned up branch to my fork, sadly the tagging format |
|
Thanks for the input and the code review, @edannenberg. Your exposure of some inconsistencies is definitely an eye-opener but as you said I think there won't be many more 1.x releases in the future but we obviously need to take more time reviewing these PRs. Do you have any tips on applying the upstream patches? I'm not sure what method is being used currently but it probably gets more difficult the further we diverge. Perhaps it should be divided into multiple PRs? For example one for /app/code, one for /app/design and one for everything else (this could vary based on where most of the changes were)? These three PRs could be against a new temporary branch like I agree that for this project squash and merge makes sense even though it goes against the concept most of us have that git commits should be preserved like ancient relics. If we could come to a consensus on this I propose that we just start using squash and merge going forward and leave the existing history intact for people that are already using branches even though a clean rebase like your 1.9.3.6 branch presents a much cleaner history. Regarding commit messages if we are using Squash and Merge then it can simply be the responsibility of the person who does the merge to clean up the commit message to meet whatever guidelines we decide upon. I think it is not realistic to expect every contributor to write perfect commit messages and it is more hassle to ask them to correct them than it is to change them during the Squash and Merge operation. So, is anyone still opposed to using squash and merge? Please state your case. @LeeSaferite has that 100% opposition dropped any since your last input? :) |
|
@colinmollenhour I still don't like squashes, but I'm just one voice in the crowd. You and @drobinson and @Flyingmana have an equal say in the matter. I've been happy that you guys are stepping up as I've been unable to work on the project much lately due to a job change. I'll leave the ultimate decision on what path we take to a consensus vote among you three. |
This repo is a bit of a special case as the goal is essentially to provide a set of patches on top of a vanilla Magento, so personally I would start with an empty branch for every Magento version and This approach assumes that the Magento core is just a runtime dep and probably won't fly well for people that use this repo as upstream for their actual project repos. Coming from the Java world this always felt horribly wrong to me. Alternatively I would recommend to at least do a full directory compare as I outlined above before merging any big upstream PR. This again requires a clean git history to be feasable.
I'm not sure breaking down the PR solves the underlying issue, it may help a bit in terms of reviewing but in the end it's still fairly large commits where you have to take extra care to not break any existing patches.
I can't remember a single time where not squashing a PR's commits before merging brought me any value, scattered feature commits on the other hand.. For example when a PR is reviewed and changes are committed weeks later (which is fine), sprinkle in some other PR merged at the same time and its already starting to get messy for other people to see what is what.
I would favor a clean cut: new branch, cleaned up history according to new commit guidelines. Leaving the old messy history may safe some users a bit of an annoyance, but on the other hand it makes getting an overview that much harder then it should be for any new user in the future. As I have just been there, it wasn't much fun to unravel the current history. Most of the work required for a clean history is already done in my branch, just needs a bit more work on the commit messages.
I fully agree. |
|
Iam also against squashing in general (it may be usefull in some cases, and I just did it today at my work project). Also I will make one thing clear: keeping the needed work for this project for both, Maintaining, and also Contributing as low as possible has the absolute highest priority. If someone really wants to set the requirements for this to a higher level,(because this is a requirement for their company?) then there are three options.
If this can not be fullfilled, then I suggest to put the changelog onto the github pages just beeing autogenerated out of the git log and later additional secondary sources to complement insufficient data from the git log. |
There are always exceptions to a rule, if it makes sense not to squash then don't. So to be more precise, PRs should not always be squashed, but useless "fixed typo", etc commits should be squashed and whatever commits get merged should be atomic in scope. That burden is on the maintainer, a good habit for any maintainer to learn though.
The point of atomic commits is to give users of this repo a decent base to work with. Imagine you just stumbled about this repo, it sounds like a really good idea. But what exactly are you buying into here? This is currently very hard to gauge, in large part due to the git history being a pretty big mess. This project is not a random Magento extension you can review quickly and move on. The large upstream merges in the past make the review process pretty much insane. So what are your options? You either say nope and continue to use vanilla Magento with your own patch set or painfully unravel the whole mess to gain the needed confidence using magento-lts on your production servers.
I'd argue that the proposed changes have minimal time impact for the maintainers, well maybe except for finding a good commit message. :) It does however require some discipline until it becomes second nature. In my experience a clean atomic git history is an asset for any repo user, while a messy one makes life just harder for no reason other having saved the maintainer a minute here and there in the past.
I'm using the squash-into-atomic process for a while now for the open source projects I maintain, the time investment is minimal and more then justified for what you get in return: a repo that is a pleasure to work with. If you had to choose, what you would rather work with: https://github.com/OpenMage/magento-lts/commits/1.9.3.x It would be trivial to get a fully automated and accurate changelog from the clean history in my fork: Anyways, just my 2c. I'm fine with cherry picking a few commits now and then into the cleaned up fork. |
|
Reviewing a commits history via github is not really the recommended way. I prefer an optimized tooling for this which brings me in the first draft to this output https://gist.github.com/Flyingmana/7074e8c3d89b8ccab1c7371ab4c0d9e9 I thought about throwing out the merges as you see, but they contain the PR number and are therefore usefull, so I added another version with them kept in. Yes, yours looks better, but if I had the choice, I would take the one with the non-squashed commits. To often I had do debug into problems, which were already in the code for months, and the original author either not reachable, no time, or just did not know anymore what in detail happens there. So if I exaggerate this, I could say squashing commits is actually harmful for the longterm maintenance of a project |
|
Just few notes from my experience as a core team member of TYPO3 - a top CMS in DACH market, which has 150-200 commits per month and 30-50 contributors per month (many of who are first time contributors). We also maintain 3 versions(branches) in parallel. How TYPO3 does itTake a look here: It's is super easy for everybody to understand what given commit is doing
We squash every branch so it finally lands as a single change in the product branch. If you want to see the evolution of the patch, you can always find it in the pull request history. But to be honest it's you need it very rarely. There are also some other rules, like - every breaking change, deprecation or a new feature requires a simple documentation file like this TYPO3/typo3@fe563ff A core developer who is merging the change is responsible of checking if the commit message is correct and describe the change in a correct way. It is neither hard nor time consuming. GoalsThe changelog/git history and the readme should:
Proposal for MageLTSThis project is of course smaller and has less contributors than TYPO3, but I think we can adapt some of the concept with big benefit for us.
|
This is the only point I'm not sure if I agree on (applying all fixes over a new branch). Users should be able to maintain their own branches and pull in upstream changes easily and I'm not sure how this works with the suggestion made. Also, there will be conflicts with upstream and I think the task of resolving these conflicts each time a new upstream version is released will be a lot of work and prone to errors each time they are resolved. That is, once a conflict has been resolved I don't see why we'd want to have to resolve it ever again. If there is ever another "major" upstream release (e.g. 1.10.0.0 - I don't expect there to be) then at this time we can decide if we should start a new 1.10.0.x branch with all patches rebased on it, but I think for now we should focus on maintaining the 1.9 branch in the simplest way possible. @Flyingmana Since we are using Github the full history is available in the Pull Request and doesn't need to be in the main branch history. Case in point, this PR resulted in 2 lines changed and 7 commits and although I squashed it when I merged it, you can still go back to the PR that is referenced in the commit message and see the full discussion and 7 commit history in all it's sloppy glory: https://github.com/OpenMage/magento-lts/pull/388/commits |
While it would be the cleanest and safest way to handle the upstream process you certainly have a point. I think what @tmotyl meant was to use this as verification before a larger upstream commit is merged into 1.9.3.x. Especially if there were any conflicts.
Yep, as I said previously, for this to be feasible a clean git history is pretty much required. The heavy lifting for this has been done in my fork and I will be able to maintain it as part my of work, so this could be used for such occasions. Maintaining the clean fork/branch could also be automated with webhooks and a bit of shell scripting if patches/upstream commits are tagged accordingly on 1.9.3.x, just cherry pick and push. For a new Magento release the process boils then down to (using 1.9.3.7 as example): Then just run meld, diff or whatever to see if Doing this process before applying the upstream changes to the 1.9.3.x branch should also give a heads up if extra care is needed not to break any patches while doing the merge. If the patch set can be applied without conflicts the merge on 1.9.3.x can be treated as one way: |
Are you saying that at the end of creating the new 1.9.3.7 branch you would merge this new branch into 1.9.3.x? There are many ways to do this so please give a specific example of what you recommend. E.g. is it something like: I've not tried the above but I think it would take the result from your ground-up method and apply it as a new commit to the existing branch. |
|
I'm not dogmatic on the point 5), probably we need to battle test it to see whether our assumptions are correct. Having a clean git history also allows us to spot bugfixes which were also fixed in Vanilla Magento but in different way (differnt place) - in that case we would probably need to remove (skip) our patch. |
|
After taking some more time to think about it, and also realizing this Topic is now already going on for a year, I suggest we should find a way to start having a changelog and improve it incrementally then. Also I will change my position on the Topic of stashing. If we notice in another year, that it is not working out we have at least proper reasons to refer to. And overall it seems here are enough people with more experience then I have on this topic, therefore I should step back a bit and not hinder you. Also I have to apologize, my last comments were to harsh and could be seen as harmful, Iam sorry for this. |
Yep. Anything i till better the nothing :P |
Sorry for being unclear, no I wouldn't merge the diff directly into
As of
In terms of automating the "patch set" branch this would be guaranteed to work as both branches should be at all times identical. The webhook just cherry-picks everything but upstream merge commits from If a new Magento version is released a new branch is started for the patch set. I'm not sure this part can be fully automated, but worst case shouldn't be more than executing a shell script Regarding the change log, if a clean branch is maintained it could be used to generate the change log from scratch, the webhook maintaining the branch could also take care of pushing the updated change log to
So let's start with defining how commit message formats should look like and get this thing rolling, proposal:
Project related commits like updating the
|
|
Just a note to commit message format ...
see: https://help.github.com/articles/closing-issues-using-keywords/ How about ... |
Ah, good point, didn't think of that. It's not really relevant for the change log though and more importantly steals precious space from the commit message. How about adding that kind of stuff below the commit details instead?
My reasoning here was that there is most likely always a PR, but not always an issue for each PR. It may also happen that a PR has no reference to it's issue. Fine with either though. For bugs it would be nice to have the first Magento version that introduced it in the message header, should be optional though: |
|
Changelog was already present in the https://github.com/OpenMage/magento-lts/blob/1.9.4.x/RELEASE_NOTES.txt file, Magento later replaced the way the changelog was shows (just link an online version of the release notes) but I guest that could be a nice place to store it. |
|
This issue is 4.5 years old... it's time to decide something or close it |
|
it kind of is already decided to have a changelog. But we still need something to provide a changelog for the what happened before we started writing one for new releases |
|
Hi everyone! In my company, I asked to use some conventions like
Moreover we use the Python CLI named Changelogdir to auto-generate our changelog by CI. |
|
I like the Conventional Commits proposal, very easy to adopt, although not as easy to enforce. The changelog options both have downsides.. single file has conflicts and multi-file means moving files around to the right version (e.g. backporting is more work). Just doesn't seem like a great solution for a Github-based project. However, by adopting Conventional Commits it would be easier to script a changelog generator (surely one already exists) |
This issue was moved to a discussion.
You can continue the conversation there. Go to discussion →
Well ... why?
In my opinion the actual description does not point out what the advantages of Magento-LTS vs Vanilla Magento are. Uhhm ... OK ..."They accecpt bug fixes". Sounds not so bad, but it's not clear whats already in ...
There are
Why dont tell that - to make this project more attractive?
Cause i'm lazy i wouldn't add a CHANGLOG.md ... i would add a link to README.md that points to https://github.com/OpenMage/magento-lts/pulls?page=1&q=is%3Apr+is%3Aclosed+is%3Amerged&utf8=%E2%9C%93 and add some labels for all merged PR (bug, enhancement, patch ...)
The text was updated successfully, but these errors were encountered: