Skip to content

[New Azure Integration Pkgs] Add DataProtection blob storage implementation based on latest Azure.Storage.Blobs #18203

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 15 commits into from
Closed

[New Azure Integration Pkgs] Add DataProtection blob storage implementation based on latest Azure.Storage.Blobs #18203

wants to merge 15 commits into from

Conversation

@BrennanConroy
Copy link
Member

@BrennanConroy BrennanConroy commented Jan 8, 2020

No description provided.

@BrennanConroy BrennanConroy added area-dataprotection Includes: DataProtection api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jan 8, 2020
@BrennanConroy BrennanConroy marked this pull request as ready for review January 10, 2020 16:45
@BrennanConroy BrennanConroy requested a review from dougbu as a code owner January 10, 2020 16:45
@BrennanConroy BrennanConroy added api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jan 10, 2020
pakrym
pakrym reviewed Jan 13, 2020
View reviewed changes
pakrym
pakrym reviewed Jan 13, 2020
View reviewed changes
@pranavkm pranavkm added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Jan 13, 2020
pranavkm
pranavkm reviewed Jan 13, 2020
View reviewed changes
BrennanConroy
BrennanConroy commented Jan 13, 2020
View reviewed changes
THIRD-PARTY-NOTICES.txt
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
THE SOFTWARE.

License notice for azure-sdk-for-net
Copy link
Member Author

@BrennanConroy BrennanConroy Jan 13, 2020

Choose a reason for hiding this comment

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

cc @Pilchie

@analogrelay analogrelay changed the title Add DataProtection blob storage implementation based on latest Azure.Storage.Blobs [New Azure Integration Pkgs] Add DataProtection blob storage implementation based on latest Azure.Storage.Blobs Jan 14, 2020
@analogrelay analogrelay added the Servicing-consider Shiproom approval is required for the issue label Jan 14, 2020
analogrelay
analogrelay reviewed Jan 14, 2020
View reviewed changes
...tection/Azure.Storage.Blob/src/Microsoft.AspNetCore.DataProtection.Azure.Storage.Blob.csproj
<PackageTags>aspnetcore;dataprotection;azure;blob</PackageTags>
<IsShippingPackage>true</IsShippingPackage>
<UseLatestPackageReferences>true</UseLatestPackageReferences>
<PreReleaseVersionLabel>$(AzureSDKPreReleaseVersionLabel)</PreReleaseVersionLabel>
Copy link
Contributor

@analogrelay analogrelay Jan 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think this is working. Looking at the public CI artifacts (which perhaps are wrong) I see: 3.1.1-ci. That indicates two things to me:

  1. The .1 patch version is applied. We want these to be 3.1.0.
  2. The -preview1 label is being lost, though again, perhaps that's a problem in the public CI.

@dotnet/aspnet-build do you have context on this? The same problem likely applies to the other two PRs (#17041, dotnet/extensions#2618)

Copy link
Member Author

@BrennanConroy BrennanConroy Jan 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The -ci is expected I believe, and -preview1 should be applied in official builds.

If you look at the other already shipped packages, you'll see they don't have -ci.
Oh, that only applies to the extensions repo...

Copy link
Contributor

@dougbu dougbu Jan 14, 2020

Choose a reason for hiding this comment

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

The setup looks correct w.r.t. $(PreReleaseVersionLabel) and $(DotNetFinalVersionKind).

We want these to be 3.1.0

Need to change the $(VersionPrefix) for that. I suggest the following in this project

<VersionPrefix>$(AspNetCoreMajorVersion).$(AspNetCoreMinorVersion).0</VersionPrefix>

Copy link
Contributor

@analogrelay analogrelay Jan 14, 2020

Choose a reason for hiding this comment

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

Sounds good. To be clear, this weird versioning will only hold true until we release the .0. Once that's done, we can just clean all the up and version them ahead to 3.1.x (whatever the next x is). I'll make a note to track that for follow up after these packages go out.

Copy link
Member Author

@BrennanConroy BrennanConroy Jan 14, 2020

Choose a reason for hiding this comment

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

@dougbu Do you know how to do the same thing for Extensions? Setting VersionPrefix doesn't seem to affect the nupkg version

Copy link
Contributor

@dougbu dougbu Jan 14, 2020

Choose a reason for hiding this comment

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

I believe there's an Arcade or .NET Core SDK setting that causes it to override $(VerisonPrefix). IIRC that's set in dotnet/aspnetcore but not dotnet/extensions. Might be $(IncludeSourceRevisionInInformationalVersion).

I didn't check a 3.x Arcade SDK but their logic in 'master' for calculating $(VersionPrefix) is in tools\Version.BeforeCommonTargets.targets and seems to no-op when the property is already set and $(PrereleaseVersionLabel) is non-empty.

Bottom line, no, I'm not sure and suggest you complete the research I started 😈

@analogrelay analogrelay removed the Servicing-consider Shiproom approval is required for the issue label Jan 14, 2020
pakrym
pakrym approved these changes Jan 14, 2020
View reviewed changes
Copy link
Contributor

@pakrym pakrym left a comment

Choose a reason for hiding this comment

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

LGTM!

@analogrelay analogrelay added the Servicing-consider Shiproom approval is required for the issue label Jan 14, 2020
@analogrelay analogrelay added this to the 3.1.x milestone Jan 14, 2020
@analogrelay
Copy link
Contributor

analogrelay commented Jan 15, 2020

Packages look better now (3.1.0-ci, and I'm presuming the -ci will become -preview1 in the official builds automatically).

@analogrelay analogrelay added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Jan 16, 2020
@analogrelay
Copy link
Contributor

analogrelay commented Jan 16, 2020

Approved for March (since this is a bundle along with dotnet/extensions#2618 (comment))

@wtgodbe wtgodbe modified the milestones: 3.1.x, 3.1.3 Jan 16, 2020
@blowdart blowdart mentioned this pull request Jan 24, 2020
Closed
@analogrelay
Copy link
Contributor

analogrelay commented Feb 13, 2020

Closing as these changes are moving.

@dougbu dougbu deleted the brecon/storage branch May 18, 2020 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JunTaoLuo JunTaoLuo Awaiting requested review from JunTaoLuo

4 more reviewers

@analogrelay analogrelay analogrelay approved these changes

@pranavkm pranavkm pranavkm left review comments

@dougbu dougbu dougbu left review comments

@pakrym pakrym pakrym approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

api-approved API was approved in API review, it can be implemented area-dataprotection Includes: DataProtection Servicing-approved Shiproom has approved the issue

Projects

None yet

Milestone

3.1.3

Development

Successfully merging this pull request may close these issues.

7 participants

@BrennanConroy @analogrelay @pranavkm @pakrym @dougbu @wtgodbe