Prompt for resources with optional resourceType #4535
+115
−0
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
heaths
requested review from
vhvb1989,
hemarina and
weikanglim
as code owners
|
Alternatively, maybe instead of a |
Within a template - hence only checking during parameter processing - prompting for *any* resource is practically useless. You have to know the type to use as intended e.g., an Azure Monitor resource. To even use an existing resource in `reference()` or `resourceId()` functions you need to know the type, as do you in an `existing` resource template in Bicep.
|
I decided to make That would be one problem with a general |
vhvb1989
reviewed
Nov 9, 2024
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
microsoft-github-policy-service
bot
added
the
no-recent-activity
|
@vhvb1989 you folks still interested in this one? |
microsoft-github-policy-service
bot
removed
the
no-recent-activity
vhvb1989
reviewed
Feb 4, 2025
Comment on lines
+134
to
+136
| if len(resources) == 0 { | ||
| return "", nil | ||
| } |
There was a problem hiding this comment.
I think this should return an error. Since the intention of the method is to prompt, the fact of an empty list is a blocker to do the prompting, so it should return an error to the caller and report that there's no resources to prompt for.
There was a problem hiding this comment.
There was a problem hiding this comment.
FWIW, my main thought with separating optional out was to also allow more permutations for other selector types. I've seen some APIs that have things like foo, fooOptional, bar, barOptional, etc., and the maintenance of and UX for all those is cumbersome. I like to separate "dials".
weikanglim
reviewed
Mar 21, 2025
| options := azapi.ListResourcesOptions{ | ||
| ResourceType: resourceType, | ||
| } | ||
| resources, err := p.resourceService.ListResources(ctx, p.env.GetSubscriptionId(), &options) |
There was a problem hiding this comment.
As I was implementing #4943, I realized that this listing would list resources in the subscription, in which case returning just the resource Name isn't enough to fully qualify the resource ID.
Returning resource ID is obviously more powerful, and still portable, but the Bicep referencing logic is rather messy. There isn't first-party functions that support this, so it ends up being a lot of string splitting with known indexing. An example of working code that is quite unappealing can be found here.
As @JeffreyCA kindly noted, this may be simpler when Bicep supports dedicated function parsing for resource IDs.
All this is to say that our hands are a little tied on making a nice little prompt experience here, and I'm honestly not sure which way we want to lean, but I did end up returning resource ID in #4943 because subscription-scope resources were a requirement there.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #4530