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

Adding @promotion decorator and learnMoreDocs validation #287

Merged
merged 14 commits into from
Feb 22, 2024

Conversation

yejee94
Copy link
Contributor

@yejee94 yejee94 commented Feb 21, 2024

adding a decorator called @promotion which helps us know which apiVersion for RP should be deploy to Portal.

also adding validation rule for @about.learnMoreDocs checking if the links are starting with https://

Comment on lines 23 to 26
export interface PromotionOptions {
apiVersion: string | EnumMember;
autoUpdate?: boolean;
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export interface PromotionOptions {
apiVersion: string | EnumMember;
autoUpdate?: boolean;
}
export interface PromotionOptions {
readonly apiVersion: string | EnumMember;
readonly autoUpdate?: boolean;
}

Copy link
Member

Choose a reason for hiding this comment

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

good practice so that other libraries don't modify your state

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed :) thank you

@timotheeguerin timotheeguerin enabled auto-merge (squash) February 22, 2024 19:17
@@ -63,3 +70,9 @@ model AboutOptions {
/** Set of links which can help learn more about the resource */
learnMoreDocs?: string[];
}

/** Options for promotion of ARM resource. */
model PromotionOptions {
Copy link
Member

Choose a reason for hiding this comment

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

Since it is a single data entry, it should be singular name.

Copy link

Choose a reason for hiding this comment

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

I kind of like it as it is, my thinking: it has several options inside... Promotion is singular, but it is done - could be multiple options. Does this make sense?

* The apiVersion will be used as a version to deploy to Portal.
* @param options Property options provides promotion information of the resourceType.
*/
extern dec promotion(target: Model, options: PromotionOptions);
Copy link
Member

Choose a reason for hiding this comment

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

Do you plan to always take a single version or an array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

only single version always :)

@@ -17,3 +19,8 @@ export interface BrowseOptions {
export interface marketplaceOfferOptions {
id?: string;
}

export interface PromotionOptions {
Copy link
Member

Choose a reason for hiding this comment

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

Same comments as above. Singular name

@@ -52,6 +55,72 @@ export function $browse(context: DecoratorContext, target: Model, options: Model
}
}

export function $promotion(context: DecoratorContext, target: Model, options: Model) {
const { program } = context;
validateDecoratorUniqueOnNode(context, target, $promotion);
Copy link
Member

Choose a reason for hiding this comment

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

nit: Since we are retiring @service{version} and will only rely on @versioned, it only rely on enum versions, the validation of each value provided can be greatly simplified. Only need to validate the enum specified is same as the one specified in @versioned on the namespace. Compiler will ensure the values are correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

talked to Allen offline, it is okay to keep this logic since customers are still using the service logic!

@timotheeguerin timotheeguerin merged commit b556e5a into Azure:main Feb 22, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants