Check for infra module before using infra-configuration for a project#3097
Conversation
vhvb1989
requested review from
ellismg,
hemarina,
wbreza and
weikanglim
as code owners
…fra-to-detect-local-infra
wbreza
left a comment
wbreza
left a comment
There was a problem hiding this comment.
Overall I feel like we are giving up some flexibility with this change. The azure.yaml was never intended to define defaults for provider, but the provider implementation should have the ability to define its own defaults when path/module has not been defined.
I don't really see how these change are fixing the linked issue. It feel
…fra-to-detect-local-infra
weikanglim
left a comment
weikanglim
left a comment
There was a problem hiding this comment.
I think this change is OK and doesn't make things significantly better or worse.
I wonder if prefer just centralizing the logic:
- Add
provisioning.ApplyDefaultsthat sets the default values forprovisioning.Options. This is the place we'd make changes if we ever wanted more specific implementations. A future change could be to have another interface to "apply defaults". - Update
project.goinprojectto useprovisioning.ApplyDefaults. Any other code that leverages the parsed project config should automatically inherit the right behavior.
I'm trying to move in a direction where there are not such a hard-dependencies from one package to another, @weikanglim I want to see the We do have many cases in azd with such dependencies. I wish you could slowly break them. |
That makes sense and I agree this being a possible goal, @vhvb1989. My suggestion was purely based on the observation that we have If we wanted to achieve the goals you've outlined, would it make more sense to have the infra config be defined in The main thing I would love addressed is the avoidance of duplication. The source of this bug was that we had duplicated the default logic handling of |
|
@weikanglim , I made a few more changes.
totally. But that's a big change. It might be better to use another PR for that. The type ProjectConfig struct {
RequiredVersions *RequiredVersions `yaml:"requiredVersions,omitempty"`
Name string `yaml:"name"`
ResourceGroupName ExpandableString `yaml:"resourceGroup,omitempty"`
Path string `yaml:"-"`
Metadata *ProjectMetadata `yaml:"metadata,omitempty"`
Services map[string]*ServiceConfig `yaml:"services,omitempty"`
Infra provisioning.Options `yaml:"infra,omitempty"`
Pipeline PipelineOptions `yaml:"pipeline,omitempty"`
Hooks map[string]*ext.HookConfig `yaml:"hooks,omitempty"`
State *state.Config `yaml:"state,omitempty"`
Platform *platform.Config `yaml:"platform,omitempty"`
*ext.EventDispatcher[ProjectLifecycleEventArgs] `yaml:"-"`
}IMO, I would break those dependencies by making the project package to be the one which defines all about |
…fra-to-detect-local-infra
wbreza
left a comment
wbreza
left a comment
There was a problem hiding this comment.
Added some suggestions to avoid having infra provider defaults managed at multiple levels within azd.
|
Is it possible for you to write some tests here - this feels like subtle behavior and I'd hate for us to regress it. |
@ellismg , unit tests added. Verified that:
Question for @ellismg. For a project defining infra: |
ellismg
left a comment
ellismg
left a comment
There was a problem hiding this comment.
Would be nice if we could at least remove the duplication of values across the three provisioning packages.
Thanks for taking the file path into account and fixing the underlying Aspire issue, @vhvb1989 and for the improvements to the tests!
Co-authored-by: Matt Ellis <matt.ellis@microsoft.com>
…fra-to-detect-local-infra
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSIContainerDocumentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
7 participants
The azure.yaml can define the configuration for the infrastructure that azd uses by setting a
pathandmodulewithin the infra folder.When no
pathis defined, azd usesinfraby defaultWhen no
moduleis defined, azd usesmainby default.The default values are currently defined inside the each provision provider, however, azure.yaml does not support having individual configuration per provider.
This PR moves the default values out of the provisioning provider, to be resolved by the provisioning manager, which calls the provider once the options are resolved.
We are also removing the
dario.cat/mergolibrary, currently used to Merge the provision options with the default values using reflection. It feels easier to just handle the 2 options directly instead of having the reflection and paying the extra tax of havingdefaultsbeing exported from the package just to allow the library to reflect it.The defaults within the provisioning package must exits there to ensure the package can be used from other components, however, the defaults inside this package are unique for the package.
Finally, the PR adds the provision defaults to be defined in the project package, and when
ProjectInfrastructure()method is called, it resolves the project defaults for azd. This ensure other commands using/parsing the project will get same defaults for the project without depending on the provisioning package.When the projectInfrastructure is pulled from the project, azd currently check only if the path to the infra is valid/exist and if yes, it use it without checking if the module name is in the folder.
This is problematic for projects where there might be an
infrafolder which is not managed by azd. For example, for #3015, there could be an Aspire project managed by azd next to aninfrafolder that contains other module names. Azd should not use the infra folder in that case, if the infra/module is not present.Fixes: #3015