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

chore: release 3.2.2 #839

Closed
wants to merge 3 commits into from
Closed

chore: release 3.2.2 #839

wants to merge 3 commits into from

Conversation

yoshi-automation
Copy link
Contributor

@yoshi-automation yoshi-automation commented Aug 30, 2019

🤖 I have created a release *beep* *boop*

3.2.2 (2019-09-06)

Bug Fixes


This PR was generated with Release Please.

@yoshi-automation yoshi-automation added autorelease: pending type: process A process-related concern. May include testing, release, or the like. labels Aug 30, 2019
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 30, 2019
@codecov
Copy link

codecov bot commented Aug 31, 2019

Codecov Report

Merging #839 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #839   +/-   ##
=======================================
  Coverage   95.22%   95.22%           
=======================================
  Files          11       11           
  Lines        1194     1194           
  Branches      296      296           
=======================================
  Hits         1137     1137           
  Misses         29       29           
  Partials       28       28

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 5f69a3d...7139530. Read the comment docs.

@jkwlui
Copy link
Member

jkwlui commented Sep 3, 2019

This release removed a field from an interface and can cause potential breakage for TypeScript user so I want to tread carefully here. @bcoe any thought on whether we should release this as a patch vs _?

@AVaksman
Copy link
Contributor

AVaksman commented Sep 4, 2019

To save time on research, the field in question slipped in the documentation inadvertently (was never meant to be part of it) see below.
#165 (comment)

@bcoe
Copy link
Contributor

bcoe commented Sep 5, 2019

@jkwlui @AVaksman if I understand correctly, this parameter was ignored, but we'll potentially break folks who were using the old types? if they attempted to set this parameter?

My temptation is to instead add a warning message for folks setting this parameter:

if (options.hasOwnProperty('keepAcl')) {
  // TODO: remove keepAcl from interface in next major.
  console.warn('keepAcl parameter is not supported');
}

@AVaksman
Copy link
Contributor

AVaksman commented Sep 5, 2019

this parameter was ignored, but we'll potentially break folks who were using the old types? if they attempted to set this parameter?

That's my understanding.

My temptation is to instead add a warning message for folks setting this parameter:

if (options.hasOwnProperty('keepAcl')) {
  // TODO: remove keepAcl from interface in next major.
  console.warn('keepAcl parameter is not supported');
}

👍 👆
I don't mind adding warning instead.
Sounds reasonable to me

@bcoe
Copy link
Contributor

bcoe commented Sep 5, 2019

@AVaksman awesome, let's take that approach; we should confirm that the warning message only happens if the user sets the value of keepAcl.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autorelease: closed cla: yes This human has signed the Contributor License Agreement. type: process A process-related concern. May include testing, release, or the like.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants