FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

doc: update CHANGELOG for 2020-08 release by mr-salty · Pull Request #4768 · googleapis/google-cloud-cpp · GitHub

/ google-cloud-cpp Public

doc: update CHANGELOG for 2020-08 release - #4768

Merged
mr-salty merged 6 commits into
googleapis:masterfrom
mr-salty:release
Aug 4, 2020
ConversationCommits (6)ChecksFiles changed
Merged

doc: update CHANGELOG for 2020-08 release#4768
mr-salty merged 6 commits into
googleapis:masterfrom
mr-salty:release

Conversation

mr-salty commented Aug 3, 2020 •
edited by coryan
Loading

Copy link
Copy Markdown
Contributor

This change is 

mr-salty requested a review from a team August 3, 2020 21:53
google-cla Bot added the cla: yes This human has signed the Contributor License Agreement. label Aug 3, 2020
coryan requested changes Aug 3, 2020
View reviewed changes

coryan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @mr-salty)

CHANGELOG.md, line 23 at r1 (raw file):

* fix: missing `CreateDefaultClientOptions` overload (#4677)
* feat: a request type to delete pending resumable uploads (#4617)
* fix!: incorrect type in OLM condition (#4597)

When we have breaking changes we have (so far) (a) created a section with all the breaking changes (search for **BREAKING in this file), and (b) included a longer description of the breaking change, typically something based on the full commit message.

coryan commented Aug 3, 2020

Copy link
Copy Markdown
Contributor

CHANGELOG.md, line 41 at r1 (raw file):

Quoted 4 lines of code… 
* fix: begin a `has_begin` transaction in Rollback() (#4731)
* fix: handle `ExecuteBatchDml` returning OK with no results (#4724)
* fix: use the updated transaction id in `ReadImpl` (#4722)
* fix: explicitly begin failed implicit begin transactions (#4706)

I suspect a lot of these could be summarized better, something like * fix: if the first operation of a transaction fails, do not attempt further operations in that transaction or something.

codecov Bot commented Aug 3, 2020 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #4768 into master will increase coverage by 0.98%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4768      +/-   ##
==========================================
+ Coverage   92.25%   93.23%   +0.98%     
==========================================
  Files         912      912              
  Lines       70429    70512      +83     
==========================================
+ Hits        64975    65744     +769     
+ Misses       5454     4768     -686
Impacted Files Coverage Δ
google/cloud/internal/log_wrapper.h 87.50% <ø> (ø)
google/cloud/internal/log_wrapper_test.cc 100.00% <ø> (ø)
...e/cloud/spanner/internal/database_admin_logging.cc 100.00% <ø> (ø)
...e/cloud/spanner/internal/instance_admin_logging.cc 90.90% <ø> (ø)
...loud/spanner/internal/logging_result_set_reader.cc 100.00% <ø> (ø)
...gle/cloud/spanner/internal/logging_spanner_stub.cc 82.35% <ø> (ø)
google/cloud/spanner/numeric.h 100.00% <ø> (+13.33%) ⬆️
google/cloud/spanner/value.h 94.48% <ø> (+4.82%) ⬆️
google/cloud/internal/log_wrapper.cc 100.00% <100.00%> (ø)
google/cloud/testing_util/assert_ok.cc 80.00% <0.00%> (-20.00%) ⬇️
... and 96 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d3f7d79...4c025d0. Read the comment docs.

mr-salty commented Aug 3, 2020

Copy link
Copy Markdown
Contributor Author

CHANGELOG.md, line 23 at r1 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

When we have breaking changes we have (so far) (a) created a section with all the breaking changes (search for **BREAKING in this file), and (b) included a longer description of the breaking change, typically something based on the full commit message.

I (mistakenly, apparently) thought we had stopped that based on recent history... have we really had zero breaking changes in the past 2 releases?

coryan commented Aug 3, 2020

Copy link
Copy Markdown
Contributor

CHANGELOG.md, line 23 at r1 (raw file):

Previously, mr-salty (Todd Derr) wrote…

I (mistakenly, apparently) thought we had stopped that based on recent history... have we really had zero breaking changes in the past 2 releases?

At least we thought none of the changes were breaking. None of them has a !, but you can verify that yourself. So I assume you are just double checking, and yes, I think we should continue the practice of describing breaking changes at length.

This file is us talking to our customers and describing (to the best of our abilities), what changes have taken place, and which of those may be tricky / difficult / annoying for them.

mr-salty commented Aug 3, 2020
View reviewed changes

mr-salty left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

ptal

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @coryan and @mr-salty)

CHANGELOG.md, line 41 at r1 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…
* fix: begin a `has_begin` transaction in Rollback() (#4731)
* fix: handle `ExecuteBatchDml` returning OK with no results (#4724)
* fix: use the updated transaction id in `ReadImpl` (#4722)
* fix: explicitly begin failed implicit begin transactions (#4706)

I suspect a lot of these could be summarized better, something like * fix: if the first operation of a transaction fails, do not attempt further operations in that transaction or something.

there is no single change that accomplished that, so this boils down to organizing the log per-issue rather than per-commit. I think that's a good idea for this case even if we don't do it for all cases.

mr-salty commented Aug 3, 2020

Copy link
Copy Markdown
Contributor Author

CHANGELOG.md, line 23 at r1 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

At least we thought none of the changes were breaking. None of them has a !, but you can verify that yourself. So I assume you are just double checking, and yes, I think we should continue the practice of describing breaking changes at length.

This file is us talking to our customers and describing (to the best of our abilities), what changes have taken place, and which of those may be tricky / difficult / annoying for them.

ack

mr-salty commented Aug 3, 2020
View reviewed changes

mr-salty left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Preview: https://github.com/googleapis/google-cloud-cpp/blob/21ee655cba2fd7e57de693eed407346ed7afab58/CHANGELOG.md

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @coryan)

coryan reviewed Aug 3, 2020
View reviewed changes

coryan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

There are a couple of nits left where we moved a class and then removed the class, so it looks like there are two changes (in two libraries), but only one matters: the removal. Sorry for yet another iteration, but I think we should simplify that, but maybe I am missing something and I am happy to change my mind.

Reviewed 1 of 1 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mr-salty)

CHANGELOG.md, line 52 at r2 (raw file):

  **`absl::CivilDay's` constructors are explicit (by design), where as the old
  `google::cloud::spanner::Date` 3-arg constructor was not explicit.**
* refactor!: move `spanner::Date` to common library (#4594)

nit: sorry I missed this earlier, that refactor! happened, but then we removed the Date class from the common library, in favor of CivilDay (see the previous change). Maybe remove this?

CHANGELOG.md, line 78 at r2 (raw file):

**BREAKING CHANGES**
* refactor!: use `absl::CivilDay` for "date" type (#4600)\

nit: see above, these date changes became trivial after all the refactoring.

mr-salty commented Aug 4, 2020
View reviewed changes

mr-salty left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Reviewable status: complete! all files reviewed, all discussions resolved

CHANGELOG.md, line 52 at r2 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

nit: sorry I missed this earlier, that refactor! happened, but then we removed the Date class from the common library, in favor of CivilDay (see the previous change). Maybe remove this?

Done

CHANGELOG.md, line 78 at r2 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

nit: see above, these date changes became trivial after all the refactoring.

Done. I read this as, the class was moved and then implemented using CivilDay, but I see it was actually replaced.

coryan approved these changes Aug 4, 2020
View reviewed changes

coryan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Reviewed 1 of 1 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved

mr-salty merged commit 2a0aeef into googleapis:master Aug 4, 2020
mr-salty deleted the release branch August 4, 2020 04:19
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

coryan coryan approved these changes

Assignees

No one assigned

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL