[Service Bus] [Core AMQP] Merge AmqpMessage{Header|Properties} exported from both the libraries #12091

HarshaNalluru · 2020-10-27T21:24:33Z

Issue #12004

Previously

// Service Bus
AmqpAnnotatedMessage {
 header: AmqpMessageHeader;
 properties: AmqpMessageProperties;
}
AmqpMessageHeader {}
AmqpMessageProperties {}

// Core AMQP
      Imports 
       MessageProperties as AmqpMessageProperties and 
       MessageHeader as AmqpMessageHeader
      from rhea and re-exports them 
       (Have `underscore`s in the attributes instead of camel-case for the props from rhea)
// Exports new MessageProperties and new MessageHeader too (these are similar to AmqpMessage{Header|Properties} defined in service-bus)

Updates

~~Re-exporting rhea types as RheaAmqpMessage{Header|Properties} instead of AmqpMessage{Header|Properties} from core-amqp~~ Removed these re-exports
Renamed Message{Header|Properties} to AmqpMessage{Header|properties} in core-amqp (still being exported)
Deleted the AmqpMessage{Header|properties} defined in service-bus and using AmqpMessage{Header|Properties} from core-amqp instead
Re-exporting AmqpMessage{Header|properties} from core-amqp in service-bus
Should we consider it non-breaking for core-amqp?
- API shape has been changed for core-amqp
- Event-hubs doesn't use any of the interfaces or types specified above, so it is not affected
- Service Bus V1 uses amqp-common, so not affected
- Only Service Bus V7 is affected(still in preview)

sdk/core/core-amqp/src/index.ts

sdk/core/core-amqp/review/core-amqp.api.md

ramya-rao-a · 2020-10-28T06:33:37Z

I would recommend to take this opportunity to have a v2 for @azure/core-amqp and clean up as many things as possible (not in this PR, but future PRs before Service Bus GA)

check-enforcer · 2020-10-28T23:14:01Z

This pull request is protected by Check Enforcer.

What is Check Enforcer?

Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass.

Why am I getting this message?

You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged.

What should I do now?

If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows:
/check-enforcer evaluate
Typically evaulation only takes a few seconds. If you know that your pull request is not covered by a pipeline and this is expected you can override Check Enforcer using the following command:
/check-enforcer override
Note that using the override command triggers alerts so that follow-up investigations can occur (PRs still need to be approved as normal).

What if I am onboarding a new service?

Often, new services do not have validation pipelines associated with them, in order to bootstrap pipelines for a new service, you can issue the following command as a pull request comment:
/azp run prepare-pipelines
This will run a pipeline that analyzes the source tree and creates the pipelines necessary to build and validate your pull request. Once the pipeline has been created you can trigger the pipeline using the following comment:
/azp run js - [service] - ci

…to harshan/sb/issue/merge-AMQP-interfaces

HarshaNalluru · 2020-10-28T23:26:45Z

/azp run js - servicebus - tests

azure-pipelines · 2020-10-28T23:26:55Z

Azure Pipelines successfully started running 1 pipeline(s).

sdk/core/core-amqp/review/core-amqp.api.md

sdk/servicebus/service-bus/review/service-bus.api.md

sdk/core/core-amqp/CHANGELOG.md

sdk/eventhub/event-hubs/package.json

sdk/servicebus/service-bus/src/serviceBusMessage.ts

ramya-rao-a

Please pin the version of core-amqp in event hubs and service bus packages as we are using the preview version. Other than that, this looks good.

…s not being set in the constructor

ramya-rao-a · 2020-10-29T07:13:49Z

sdk/servicebus/service-bus/src/serviceBusMessage.ts

@@ -438,7 +438,7 @@ export interface ServiceBusReceivedMessage extends ServiceBusMessage {
   * @property {AmqpMessage} _amqpMessage The underlying raw amqp message.
   * @readonly
   */
-  readonly _amqpAnnotatedMessage: AmqpAnnotatedMessage;
+  readonly _amqpAnnotatedMessage?: AmqpAnnotatedMessage;


Making this optional will not be an accurate representation of a received message. I would recommend reverting the commit a745549 and revisiting the refactoring later.

…nce it is not being set in the constructor" This reverts commit a745549.

ramya-rao-a · 2020-10-29T15:38:09Z

sdk/core/core-amqp/CHANGELOG.md

+## 2.0.0-beta.1 (Unreleased)
+
+- `AmqpAnnotatedMessage` interface that closely represents the AMQP annotated message from the [AMQP spec](https://docs.oasis-open.org/amqp/core/v1.0/os/amqp-core-messaging-v1.0-os.html#section-message-format) has been added. New `AmqpMessageHeaders` and `AmqpMessageProperties` interfaces(properties with camelCasing) have been added in the place of re-exports from "rhea" library(properties with snake_casing).
+  [PR 12091](https://github.com/Azure/azure-sdk-for-js/pull/12091)


Love the changelog update. Its perfect :)

ramya-rao-a · 2020-10-29T15:40:28Z

sdk/core/core-amqp/review/core-amqp.api.md

@@ -8,13 +8,13 @@ import { AbortSignalLike } from '@azure/abort-controller';
 import { AccessToken } from '@azure/core-auth';
 import { AmqpError } from 'rhea-promise';
 import { Message as AmqpMessage } from 'rhea-promise';


Nit: Can we keep this as Message and not use the Amqp prefix here? Everywhere else where we use the Amqp prefix, we are referring to the type/interface defined in core-amqp which has camel casing. Where as this AmqpMessage is from rhea-promise. To be consistent with MessageHeader and MessageProperties from rhea-promise, this should just Message as well

We either do that or use the Rhea prefix

I was thinking it, but refrained from doing since it would change service-bus and event-hubs.
But it is probably worth doing it.. will do!

ghost · 2020-10-29T21:44:18Z

Hello @HarshaNalluru!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

HarshaNalluru added 6 commits October 27, 2020 14:20

Amqp{Header|Properties} -> RheaAmqp{Header|Properties}

4e20516

Api report and fix build failures

9b8b905

reuse core-amqp types

36ca283

API report - service-bus

7a7b701

fix build failures

8a4e6d4

API Report

bcf7a0e

ghost added Azure.Core Service Bus labels Oct 27, 2020

HarshaNalluru commented Oct 27, 2020

View reviewed changes

sdk/core/core-amqp/src/index.ts Show resolved Hide resolved

HarshaNalluru commented Oct 27, 2020

View reviewed changes

sdk/core/core-amqp/src/index.ts Outdated Show resolved Hide resolved

ramya-rao-a reviewed Oct 28, 2020

View reviewed changes

sdk/core/core-amqp/review/core-amqp.api.md Show resolved Hide resolved

sdk/core/core-amqp/review/core-amqp.api.md Outdated Show resolved Hide resolved

sdk/core/core-amqp/review/core-amqp.api.md Outdated Show resolved Hide resolved

HarshaNalluru added 3 commits October 28, 2020 16:03

Export AmqpAnnotatedMessage from core-amqp

30f16f1

remove AMQPAnnotatedMessage from exports - API Report

11f412d

Do not export RheaAmqpMessageHeader and RheaAmqpMessageProps

f510cae

HarshaNalluru added 3 commits October 28, 2020 16:17

API report and fix build failures

ef92276

Merge branch 'master' of https://github.com/Azure/azure-sdk-for-js in…

779aa91

…to harshan/sb/issue/merge-AMQP-interfaces

API Report

132d942

HarshaNalluru marked this pull request as ready for review October 28, 2020 23:24

HarshaNalluru requested review from bterlson, chradek, richardpark-msft and xirzec as code owners October 28, 2020 23:24

HarshaNalluru requested a review from ramya-rao-a October 28, 2020 23:25

ramya-rao-a reviewed Oct 28, 2020

View reviewed changes

sdk/core/core-amqp/review/core-amqp.api.md Outdated Show resolved Hide resolved

ramya-rao-a reviewed Oct 28, 2020

View reviewed changes

sdk/core/core-amqp/review/core-amqp.api.md Show resolved Hide resolved

ramya-rao-a reviewed Oct 28, 2020

View reviewed changes

sdk/servicebus/service-bus/review/service-bus.api.md Outdated Show resolved Hide resolved

changelog and version update

6392a44

HarshaNalluru mentioned this pull request Oct 29, 2020

[Core AMQP] Version 2 and its breaking changes #12116

Closed

9 tasks

HarshaNalluru added 4 commits October 28, 2020 17:38

remove AmqpHeader and AmqpProps from the exports in index.ts

b0b94a6

API Report

35a38ec

core-amqp version update in service-bus

43f0396

update core-amqp for event-hubs

ad6da86

ramya-rao-a reviewed Oct 29, 2020

View reviewed changes

sdk/core/core-amqp/CHANGELOG.md Show resolved Hide resolved

ramya-rao-a reviewed Oct 29, 2020

View reviewed changes

sdk/eventhub/event-hubs/package.json Outdated Show resolved Hide resolved

ramya-rao-a reviewed Oct 29, 2020

View reviewed changes

sdk/servicebus/service-bus/src/serviceBusMessage.ts Outdated Show resolved Hide resolved

ramya-rao-a approved these changes Oct 29, 2020

View reviewed changes

HarshaNalluru added 5 commits October 28, 2020 23:35

Move changes to v2

37ed73a

pin beta versions

6786692

Make _amqpAnnotatedMessage optional in the ReceivedMessage since it i…

a745549

…s not being set in the constructor

changelog for service-bus and API report and fix build failures

e87d659

Changelog for core-amqp v2

dfe570a

HarshaNalluru requested a review from ramya-rao-a October 29, 2020 07:10

ramya-rao-a reviewed Oct 29, 2020

View reviewed changes

HarshaNalluru added 5 commits October 29, 2020 00:15

Amqp spec link

ba70742

Revert "Make _amqpAnnotatedMessage optional in the ReceivedMessage si…

1f0cde3

…nce it is not being set in the constructor" This reverts commit a745549.

remove bang(!)

8738f03

http -> https

cf1eab8

API Report

e90c03f

HarshaNalluru requested a review from ramya-rao-a October 29, 2020 09:04

ramya-rao-a reviewed Oct 29, 2020

View reviewed changes

ramya-rao-a approved these changes Oct 29, 2020

View reviewed changes

HarshaNalluru added the auto-merge label Oct 29, 2020

ghost merged commit 1d5a5d6 into Azure:master Oct 29, 2020

ramya-rao-a mentioned this pull request Oct 29, 2020

[Service Bus] Amqp Message Properties and Header defined both in Service Bus and core-amqp #12004

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Service Bus] [Core AMQP] Merge AmqpMessage{Header|Properties} exported from both the libraries #12091

[Service Bus] [Core AMQP] Merge AmqpMessage{Header|Properties} exported from both the libraries #12091

HarshaNalluru commented Oct 27, 2020 •

edited

Loading

ramya-rao-a commented Oct 28, 2020

check-enforcer bot commented Oct 28, 2020

HarshaNalluru commented Oct 28, 2020

azure-pipelines bot commented Oct 28, 2020

ramya-rao-a left a comment

ramya-rao-a Oct 29, 2020

HarshaNalluru Oct 29, 2020

ramya-rao-a Oct 29, 2020

ramya-rao-a Oct 29, 2020

ramya-rao-a Oct 29, 2020

HarshaNalluru Oct 29, 2020

ghost commented Oct 29, 2020

[Service Bus] [Core AMQP] Merge AmqpMessage{Header|Properties} exported from both the libraries #12091

[Service Bus] [Core AMQP] Merge AmqpMessage{Header|Properties} exported from both the libraries #12091

Conversation

HarshaNalluru commented Oct 27, 2020 • edited Loading

Issue #12004

Previously

Updates

ramya-rao-a commented Oct 28, 2020

check-enforcer bot commented Oct 28, 2020

What is Check Enforcer?

Why am I getting this message?

What should I do now?

What if I am onboarding a new service?

HarshaNalluru commented Oct 28, 2020

azure-pipelines bot commented Oct 28, 2020

ramya-rao-a left a comment

Choose a reason for hiding this comment

ramya-rao-a Oct 29, 2020

Choose a reason for hiding this comment

HarshaNalluru Oct 29, 2020

Choose a reason for hiding this comment

ramya-rao-a Oct 29, 2020

Choose a reason for hiding this comment

ramya-rao-a Oct 29, 2020

Choose a reason for hiding this comment

ramya-rao-a Oct 29, 2020

Choose a reason for hiding this comment

HarshaNalluru Oct 29, 2020

Choose a reason for hiding this comment

ghost commented Oct 29, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

HarshaNalluru commented Oct 27, 2020 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.