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

Create changes file #221

Merged
merged 3 commits into from
Sep 22, 2022
Merged

Create changes file #221

merged 3 commits into from
Sep 22, 2022

Conversation

Jessie212
Copy link
Contributor

No description provided.

@cla-bot cla-bot bot added the cla-signed label Aug 16, 2022
@Jessie212 Jessie212 requested a review from ebyhr August 16, 2022 20:53
@Jessie212
Copy link
Contributor Author

@ebyhr What would you like me to do about the previous versions? Should we just mention that features from all the releases are documented in the README?

CHANGES.md Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
Copy link
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

Minor add but overall good to go.

Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

I don't think there's a process proposed here so just adding this file won't help with maintaining release notes.

There's no point in listing past releases. It's not useful use of time to vet what changes went in past releases.

One alternative is something like in https://github.com/trinodb/charts/releases/tag/trino-0.8.0 where anything not explicitly labelled gets added to release notes automatically. It can then be later hand-edited as needed.

@mosabua
Copy link
Member

mosabua commented Aug 17, 2022

We are adding this file as suggested by you since there was no other release notes list. We pulled the info from the announcements on slack. The proposed process changes is to just update this file prior to each release manually. Then each tag has the latest info in it and master branch also has it readily available. We can then link to this document from other sources that want to refer to specific releases or the changes in general.

@hashhar
Copy link
Member

hashhar commented Aug 17, 2022

So something similar to how https://github.com/airlift/airbase handles release notes. Every PR that is a user facing change also adds changes to CHANGES.md? This makes sense to me.

The Slack announcements are a cherry-picked listing of "some impactful" changes - they aren't exhaustive and listing them in a formal changelog would be slightly misleading.

We can start with current release.

@hovaesco
Copy link
Member

FYI we recently introduced changie in dbt-trino starburstdata/dbt-trino#98 (and other useful stuff like linting, formatting etc.). It automates creating of CHANGELOG.

@leniartek
Copy link

@mosabua Going forward we will automate change log creation as we did in dbt-trino https://github.com/starburstdata/dbt-trino/blob/master/CHANGELOG.md

I don't mind adding the past release notes manually if you want to.

Screenshot 2022-08-18 at 14 01 16

@mosabua
Copy link
Member

mosabua commented Aug 18, 2022

okay ... then we can close this PR maybe? Either way .. just let us know when that page is up..

@hashhar
Copy link
Member

hashhar commented Aug 18, 2022

@leniartek That release notes page has some issues - the people who contribute changes shouldn't be the one contributing release notes. It needs an editorial process. As an example none of the "features" mentioned in the release notes mention how to opt into or use that feature. Same for bugs - they don't say when or how could it be encountered.

We should involve @mosabua / @colebow for the initial process and leverage their help to see what parts can and cannot be automated.

@mosabua
Copy link
Member

mosabua commented Sep 7, 2022

Is there any progress on how you want to proceed @leniartek @hovaesco @hashhar and others?

@hashhar
Copy link
Member

hashhar commented Sep 8, 2022

@mosabua I'm in favour of the approach proposed here because the people writing release notes should not be same as the one who added the feature since it leads to pointless changelogs (see the example in my comment above).

@hashhar
Copy link
Member

hashhar commented Sep 8, 2022

I'll merge this PR but I still think we should not document past releases since those release notes are not complete and only a highlight of select features from a release.

@hovaesco
Copy link
Member

hovaesco commented Sep 8, 2022

I would argue that it leads to pointless changelogs since it comes within PRs, so maintainer can review it, suggest rewording, propose something different etc.

I'm fine with whatever approach which will improve the quality of delivering new releases. We all see a need of having release notes, so let's just figure out who will be responsible for maintaining them and what process we can follow.

@hovaesco
Copy link
Member

I've opened a PR which adds a pull request template, since we should follow the same process as in Trino project. #234

@mosabua
Copy link
Member

mosabua commented Sep 13, 2022

So I think we need to decide how to proceed. If we follow the same approach as in trino, then we would merge the PR template, merge this PR, and then create a PR that updates the info for the next release. Then keep updating that PR and merge it just before release.

We are happy to help with this implementation just like we do in Trino .. if you are happy to go down that route.

One thing we would have to decide is if we use this current content and just get better... or if we start with zero info. Personally I think some release notes (like in this PR) are better than none .. but YMMV

@hashhar
Copy link
Member

hashhar commented Sep 13, 2022

So I think we need to decide how to proceed. If we follow the same approach as in trino, then we would merge the PR template, merge this PR, and then create a PR that updates the info for the next release. Then keep updating that PR and merge it just before release.

Yes. That makes sense. We want to start a process similar to Trino for now and we can see what can be improved over time.

We are happy to help with this implementation just like we do in Trino .. if you are happy to go down that route.

Thank you.

One thing we would have to decide is if we use this current content and just get better... or if we start with zero info. Personally I think some release notes (like in this PR) are better than none .. but YMMV

I tend to think that starting with no information is better since the current release notes are just some selected highlights and slightly misleading (since they are not complete). But I'm with either.

@hashhar
Copy link
Member

hashhar commented Sep 14, 2022

Ping @colebow to acknowledge since we'd need his help as well. I'll merge this then.

@colebow
Copy link
Member

colebow commented Sep 14, 2022

Ping @colebow to acknowledge since we'd need his help as well. I'll merge this then.

Happy to help in whatever way is needed.

@hashhar
Copy link
Member

hashhar commented Sep 21, 2022

Thanks @colebow. I'm merging this PR - the process is similar to what we do in Trino.

  • Each PR (at-least recently opened ones) will have a release notes in PR template.
  • We keep a PR open with draft release notes - in CHANGES.md added in this PR
  • Before a release is cut we review and merge CHANGES.md
  • Perform a release and announce.

CHANGES.md Outdated
Comment on lines 10 to 16
* Add support for SQLAlchemy queries to access multiple catalogs by specifying
a `trino_catalog` argument to SQLAlchemy `Table` objects. (#186)
* Improve performance when a cursor with `experimental_python_types` is used.
(#206)
* Fix incorrect results for `get_table_comment` in SQLAlchemy when two tables
having same name and schema exist in different catalogs. (#217)
* Remove spurious logging of HTTP responses when query is cancelled. (#216)
Copy link
Member

Choose a reason for hiding this comment

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

I added these for 0.316.0.

Copy link
Member

Choose a reason for hiding this comment

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

@colebow can you review this please.

CHANGES.md Outdated Show resolved Hide resolved
Copy link
Member

@colebow colebow left a comment

Choose a reason for hiding this comment

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

Two small nits, otherwise LGTM.

CHANGES.md Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
@mosabua
Copy link
Member

mosabua commented Sep 21, 2022

Can we get this merged and then do an update to the new release in a follow up PR? @Jessie212 is unavailable this week

@hashhar hashhar merged commit 7cb5deb into trinodb:master Sep 22, 2022
@hashhar
Copy link
Member

hashhar commented Sep 22, 2022

Thanks @Jessie212 and others for contributing this and helping make sure future versions come with release notes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

6 participants