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

Bump version to 1.8.0. #417

Merged
merged 2 commits into from
Dec 19, 2023
Merged

Conversation

Copy link

codecov bot commented Nov 29, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7a6700f) 33.39% compared to head (93e421d) 33.39%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #417   +/-   ##
=======================================
  Coverage   33.39%   33.39%           
=======================================
  Files          22       22           
  Lines        3980     3980           
=======================================
  Hits         1329     1329           
  Misses       2520     2520           
  Partials      131      131           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@avas27JTG avas27JTG added this to the v1.8.0 milestone Nov 29, 2023
Copy link
Collaborator

@hanzei hanzei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to include some of the currently open PRs into the release?

@avas27JTG
Copy link
Contributor Author

avas27JTG commented Nov 29, 2023

Do we want to include some of the currently open PRs into the release?

@hanzei
I think we need to create this release soon as per discussions with @mickmister so, I think including other open items to this release will extend the timeline. However, we can plan open PRs for the next release on priority.
Let me know your thoughts.

@mickmister
Copy link
Contributor

@avas27JTG We can create a release with just #408 if we want to limit the scope of this and get it out sooner. It's not a small PR, but if it's easy to cherry-pick, maybe we should go with that to keep the release smaller.

Copy link
Collaborator

@hanzei hanzei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if #371 severe enough that we would take on the afford of releasing a version just to fix it.

@avas27JTG
Copy link
Contributor Author

Cherry pick is possible but please let me know if we should go with it or do a full release #417 (review) @mickmister @hanzei

@hanzei hanzei added the 2: Dev Review Requires review by a core committer label Nov 30, 2023
@mickmister
Copy link
Contributor

@hanzei The title of the PR is a bit misleading, as the main benefit of the PR is that we limit the number of API calls from the frontend from 4 to just 1 on websocket reconnect. Pushing this change is in theory going to help with the issues happening with intermittent logouts due to token management, see thread for discussion. This is a bit of a bandaid for the problem, but we're trying to mitigate the issue as much as we can, with what we currently have available to us.

I have experienced the issue on community twice now, though haven't seen it for a few weeks. Someone recently reported this so we know others are running into it too #411

@mickmister
Copy link
Contributor

Yeah I'm thinking we should cherry-pick that PR to minimize changes in the release. Unless it proves too difficult to cherry-pick that. There's no set timeline on this, but I think we should get it out soon if we can. Thoughts @hanzei @avas27JTG? Open to other strategies here

@avas27JTG
Copy link
Contributor Author

avas27JTG commented Dec 4, 2023

Yeah I'm thinking we should cherry-pick that PR to minimize changes in the release. Unless it proves too difficult to cherry-pick that. There's no set timeline on this, but I think we should get it out soon if we can. Thoughts @hanzei @avas27JTG? Open to other strategies here

@mickmister I will check if cherry-pick is easy here, but just a thought even by cherry-pick we will create a new version, so why not create a release with everything we have in master as of now to include more items in the release?
Also, most of the PRs merged in the master are QA-approved, so it won't be a delay considering QA sanity.

Please add your thoughts.

@mickmister
Copy link
Contributor

I will check if cherry-pick is easy here, but just a thought even by cherry-pick we will create a new version, so why not create a release with everything we have in master as of now to include more items in the release? Also, most of the PRs merged in the master are QA-approved, so it won't be a delay considering QA sanity.

@avas27JTG Mainly to keep the release smaller, though it's already pretty small besides the link tooltip feature. If we're good to ship that feature as it currently stands, then let's just ship off of master 👍

@mickmister
Copy link
Contributor

mickmister commented Dec 4, 2023

@avas27JTG Are you thinking we should wait to merge more PRs, or release what we currently have?

@avas27JTG
Copy link
Contributor Author

@avas27JTG Are you thinking we should wait to merge more PRs, or release what we currently have?

@mickmister We have some PRs pending on just QA review so, I think we should get them QA reviewed max by this and next week and cut the release including them as well by the next week, but if we need to do the release this week we can go with what we have currently.

@mickmister mickmister added 3: QA Review Requires review by a QA tester and removed 2: Dev Review Requires review by a core committer labels Dec 12, 2023
@mickmister
Copy link
Contributor

@avas27JTG I'm thinking it's good to get #371 out, if we think the release will take longer than originally expected

@avas27JTG
Copy link
Contributor Author

avas27JTG commented Dec 13, 2023

@mickmister this release is ready we are just waiting on a final QA sanity which is expected to be completed by max Friday this week i.e. 15th so, we would be able to push it on or before 15th Dec as soon as QA sanity is done.

If any bug/issue found here during the sanity, then we can take out #371 in a dot release.

@AayushChaudhary0001
Copy link

Tested and approved
All the PR's included and working fine and overall functionality works well, LGTM

@avas27JTG
Copy link
Contributor Author

avas27JTG commented Dec 14, 2023

@mickmister this release is ready we are just waiting on a final QA sanity which is expected to be completed by max Friday this week i.e. 15th so, we would be able to push it on or before 15th Dec as soon as QA sanity is done.

If any bug/issue found here during the sanity, then we can take out #371 in a dot release.

@mickmister this release is ready now with QA approval, we can proceed with next steps.

@hanzei hanzei removed the request for review from spirosoik December 18, 2023 11:51
@hanzei hanzei added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester labels Dec 18, 2023
@mickmister
Copy link
Contributor

mickmister commented Dec 18, 2023

@ayusht2810 @avas27JTG I'm thinking it's best not to bring in significant changes (e.g. link tooltip feature) after the release has been approved by QA. The defined scope of the release has changed because of this. I'm thinking we should release what has been running on community as 1.8.0, since that is what we have tested (in QA and community), and release other changes as 1.9.0. We can merge this PR with that in mind, and I'll make sure we release off of the commit before the one above when we cut the plugin.

@avas27JTG
Copy link
Contributor Author

avas27JTG commented Dec 19, 2023

@ayusht2810 @avas27JTG I'm thinking it's best not to bring in significant changes (e.g. link tooltip feature) after the release has been approved by QA. The defined scope of the release has changed because of this. I'm thinking we should release what has been running on community as 1.8.0, since that is what we have tested (in QA and community), and release other changes as 1.9.0. We can merge this PR with that in mind, and I'll make sure we release off of the commit before the one above when we cut the plugin.

Make sense, thanks @mickmister we will mention tooltip issue in v1.9.0

@avas27JTG
Copy link
Contributor Author

Removed #301 from the description of this PR as it will not be a part of this release.

@mickmister mickmister merged commit e611323 into mattermost:master Dec 19, 2023
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants