Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

r31 changelog #1407

Closed
gokcehan opened this issue Sep 2, 2023 · 13 comments
Closed

r31 changelog #1407

gokcehan opened this issue Sep 2, 2023 · 13 comments

Comments

@gokcehan
Copy link
Owner

gokcehan commented Sep 2, 2023

Changelog

@gokcehan
Copy link
Owner Author

gokcehan commented Sep 2, 2023

I think we should make a new release soon. I'm thinking of making a new release once the current active PRs are merged. So consider it a feature freeze for r31. @joelim-work @ilyagr Let me know if you need more time for anything.

I have put together a preview for the changelog (@ mentions and # links are excluded to avoid notifications). Let me know if you have any suggestions (e.g. better wording, missing item, misslink/missattribution, label change for breaking/new/fix).

From now on, I'm thinking of maintaining the changelog previews public in a pinned issue so I can update it occasionally from time to time. Otherwise, I'm having trouble motivating myself to make a new release when there are too many changes piled up. This should also be useful for users to see what's currently cooking for the new release. Let me know if you have other suggestions.

@gokcehan gokcehan pinned this issue Sep 2, 2023
@joelim-work
Copy link
Collaborator

I think having a freeze period for new releases makes a lot of sense, especially since this project follows a simple master-only branching strategy. This changelog preview is also very helpful in terms of transparency to both collaborators and advanced users who closely follow the development of the project.

This is just a suggestion, but I wonder whether it is worth maintaining a CHANGELOG.md in the repo. By treating the changelog as code itself, you receive the usual benefits from source control management (e.g. easier collaboration, pull request submissions, historical changes, etc.), and you can just copy/paste the contents when creating a new release.


Regarding the state of pull requests:

@gokcehan
Copy link
Owner Author

gokcehan commented Sep 3, 2023

@joelim-work I thought about CHANGELOG.md but I decided we might still need some place to discuss the current state of things, though we can also simply separate the discussion from the changelog. I also wasn't sure about keeping the changelog in the repo since it has github specific information (i.e. @ mentions and # links) and it might increase the commit frequency, though these are not really a big deal. In general I guess it is a good idea to move it to the repository, especially considering the collaboration aspect of it as I'm not the best person when it comes to explaining things. I will try to convince myself that it is a good idea in the upcoming days. I'm also a little hesitant to change our labeling convention to adopt the one in keepachangelog.

@joelim-work
Copy link
Collaborator

@gokcehan I forgot to reply to this earlier but the use of CHANGELOG.md is just a suggestion, you don't have to use it. For instance I have found that (as a collaborator?) I am able to edit your comments so maybe just creating an issue like this for each release is sufficient.

That being said, if you do want to add in a CHANGELOG.md, I don't think there's any need to keep it up to date all the time, you can update it once in a while and/or just before a new release. There's also not really any need to follow the exact format specified in https://keepachangelog.com/en/1.0.0/

@ilyagr
Copy link
Collaborator

ilyagr commented Sep 5, 2023

I haven't had any time for PR review this weekend. My brother got married. 🎉

I'm mildly worried about the complexity of server-client interaction that was added in #1384, but I am mostly worried about documenting this in places where someone might try, say, to send a message to the server over a channel that the server is no longer listening to. I haven't followed the entire discussion that lead to the design of #1384, but I don't expect to come up with something better in terms of the overall design. There's always the option of merging that PR and making any changes, if necessary, in follow-up PRs.

@joelim-work
Copy link
Collaborator

I haven't had any time for PR review this weekend. My brother got married. 🎉

No worries at all, my congratulations! 🎉

I do acknowledge that there is some more complexity in the interaction between the server and the client. Up until now, the communication was effectively one way - the server forwards send commands to the client(s) to execute. With the addition of query, the client can now send data back to the server, which should work if only one server thread is reading from the socket (the thread that is handling the current query request). This is why the thread handling the original conn request has to return - it was not doing anything useful anyway.

To be clear, the original design was actually proposed by @gokcehan in #1310 (comment), and I just merely got it to work, and then further built upon that idea.

In terms of what to do, the discussion starting from #1277 has already lasted a few months, I am not in a rush to merge this and I don't mind giving you more time if you want. This PR can even skip r31 if you feel it to be necessary. Otherwise I also don't mind merging this and dealing with any issues afterwards.

P.S.: I'm not sure if the release notes need to clarify this, but to use the new query command, the server has to be restarted after upgrading.

@gokcehan
Copy link
Owner Author

gokcehan commented Sep 7, 2023

@ilyagr @joelim-work I looked at #1384 again now and it looks good to me. Since the new code path with query is partly separate, the changes are even more limited. So I don't really expect any issues with the previous server commands. If anything, maybe there could be some issues with the new query command, though that should be acceptable. To be honest, I don't remember many contributions to the server-client before, so it probably consists of my quick and dirty solutions and I wouldn't really trust them much at this point. If there is still a possibility to replace #1384 with one the alternative approaches proposed in #1277, then we should skip it for r31. Otherwise, I don't see much point in waiting further, so I'm in favor of including it in r31. Let me know what you decide.

@ilyagr Congratulations to your brother. Speaking of irl, I'm in the process of changing my life setup these days, and I will probably disappear for a while towards the end of this month, hopefully not permanently. This is why I want to push a new release soon, preferably this weekend. Looking at the changelog, it is appearent that @joelim-work is handling most of the development these days, and I only do occasional complaints about breaking changes, so I don't expect any issues when I'm not around.

@joelim-work I forgot to think about the changelog, but since you can also edit the comment maybe we should just keep it as an issue for now. I have now added a breaking change about the server restart.

@joelim-work
Copy link
Collaborator

@gokcehan I've decided to merge #1384, I think we might as well not wait any longer. Regarding all the other alternative approaches discussed over the last few months, I consider them to be inferior to the query approach, and I just merely kept them for reference.

I have merged the Upcoming Changes section into the Unreleased Changes section. I think I would prefer if you wait for another week before releasing r31 just in case, but I don't think there will be any issues. I should be around during this time to respond to issues if anything comes up.

@ilyagr
Copy link
Collaborator

ilyagr commented Sep 8, 2023

I'm entirely fine with merging #1384. I still hope to look it over, but I never meant for that to slow everything down so much; I can always make follow-up PRs if I will feel like it. Sorry, I continue to have very limited free time and, more importantly, brainpower for lf. I hope that changes in the near future, but I can't make any promises.

@gokcehan, feel free to release whenever works for you. I'd also prefer that to be later (e.g. in a week as Joe suggested) rather than sooner, but it's more important that it works with your schedule.

@gokcehan
Copy link
Owner Author

gokcehan commented Sep 9, 2023

@joelim-work @ilyagr Waiting for an extra week sounds good.

@tayopi
Copy link

tayopi commented Sep 12, 2023

In the mean time, is there a workaround for #1371?

@joelim-work
Copy link
Collaborator

In the mean time, is there a workaround for #1371?

You can always build from source, which will give you access to all the latest bug fixes and features.

@gokcehan gokcehan changed the title r31 changelog (preview) r31 changelog Sep 17, 2023
@gokcehan
Copy link
Owner Author

r31 is released today. Thanks to all contributors.

@gokcehan gokcehan unpinned this issue Sep 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants