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

Remove provision provider initialization from 'up' command #3988

Closed
wants to merge 1 commit into from

Conversation

wbreza
Copy link
Contributor

@wbreza wbreza commented Jun 10, 2024

Resolves #3973

Previously we were initializing the provision provider to collect the subscription and location information early within the command.

Since the up command now executes a workflow object - this preemptive initialization causes issues with templates that leverage preprovision hooks to set any required azd environment variables used as defaults of infrastructure parameters causing end users to incorrectly be prompted to supply a value.

Pros*

  • Required prompts fire at the correct time within a up workflow

Cons

  • Subscription / location are not collected upfront but rather at provision time

Copy link
Member

@vhvb1989 vhvb1989 left a comment

Choose a reason for hiding this comment

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

I don't think we can just remove the Initialize().

We need to take some time to review #3745 with PM (due to the UX implications). And also think/explore if there are other side effects (I'm worry about resource-group-deployment)

@@ -108,19 +88,6 @@ func (u *upAction) Run(ctx context.Context) (*actions.ActionResult, error) {
}
defer func() { _ = infra.Cleanup() }()

// TODO(weilim): remove this once we have decided if it's okay to not set AZURE_SUBSCRIPTION_ID and AZURE_LOCATION
// early in the up workflow in #3745
err = u.provisioningManager.Initialize(ctx, u.projectConfig.Path, infra.Options)
Copy link
Member

Choose a reason for hiding this comment

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

IIRC, this was required to support resource group deployments. Azd needs to compile any existing bicep and figure out not only the prompt for parameters, but picking or creating a resource group.

IIRC, we need to break the Initialize method from the provisioning manager in more than one only Initialize method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Anything that would happen will still happen - just in the provision command. I will test resource group deployments to validate.

Copy link
Contributor

Hi @wbreza. Thank you for your interest in helping to improve the Azure Developer CLI experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days.

@microsoft-github-policy-service microsoft-github-policy-service bot added the no-recent-activity identity issues with no activity label Aug 16, 2024
Copy link
Contributor

Hi @wbreza. Thank you for your contribution. Since there hasn't been recent engagement, we're going to close this out. Feel free to respond with a comment containing "/reopen" if you'd like to continue working on these changes. Please be sure to use the command to reopen or remove the "no-recent-activity" label; otherwise, this is likely to be closed again with the next cleanup pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-recent-activity identity issues with no activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

azd env set in a hook script doesn't have the variables available in future steps
2 participants