-
Notifications
You must be signed in to change notification settings - Fork 16
[FEATURE] Get subscription id in spk setup command #386
Conversation
*/ | ||
export const getSubscriptions = ( | ||
rc: IRequestContext | ||
): Promise<ISubscriptionItem[]> => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not using async
since the entire code base using it? Promise
is used in few places due to SDK dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you show me how you want the code to be written? async keyword is needed when we have await in the function; and wrapping the entire function as a Promise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is already returning promise
which means you are using asynchronous or blocking code in the function. async
functions use an implicit Promise to return results anyway. Keeping code consistent would be good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my point is we do not add async unnecessary and it is not encourage. it is not about consistency.
Sorry I do not get your point on why you want to add async?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can write the function in the manner but not simply adding an async keyword.
export const getSubscriptions = async (
rc: IRequestContext
): Promise<ISubscriptionItem[]> => {
logger.info("attempting to get subscription list");
const creds = await loginWithServicePrincipalSecret(
rc.servicePrincipalId!,
rc.servicePrincipalPassword!,
rc.servicePrincipalTenantId!
);
const client = new SubscriptionClient(creds);
const subsciptions = await client.subscriptions.list();
const result = (subsciptions || []).map(s => {
return {
id: s.subscriptionId!,
name: s.displayName!
};
});
logger.info("Successfully acquired subscription list");
return result;
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look good overall. Pending the comments from others
In the event that there are more than one subscription, user needs to select one of them. Display names shall be displayed as choices.
the subscription id value will be written to spk config.yaml file
closes microsoft/bedrock#1138