-
Notifications
You must be signed in to change notification settings - Fork 264
Revert "Decouple project.ProjectConfig from provisioning.Options" #6198
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
base: main
Are you sure you want to change the base?
Conversation
- Created InfraConfig type in project package as data-only representation - Added ToProvisioningOptions() and InfraConfigFromProvisioningOptions() conversion functions - Updated ProjectConfig and ServiceConfig to use InfraConfig instead of provisioning.Options - Updated all call sites to convert when provisioning.Options is needed - Fixed layer provider assignment to preserve custom values - Removed unused provisioning imports from project package - Added comprehensive tests for conversion functions Co-authored-by: vhvb1989 <24213737+vhvb1989@users.noreply.github.com>
Co-authored-by: vhvb1989 <24213737+vhvb1989@users.noreply.github.com>
wbreza
left a comment
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.
Provisioning is a core aspect of azd and part of any project. There is no value in duplicating the configuration into the project package. There also many other references to the provisioning package through the project and other packages. This change is not DRY and violates single responsibility principal.
I’d prefer that we keep a single canonical config type per concern (project, provisioning, state, etc.) and have the other packages depend on those.
Duplicating the same config structs in multiple packages means we’ll eventually see drift, more conversion code, and confusion about which version is authoritative.
@copilot ^ comment for you |
This reverts the changes that introduced InfraConfig type duplication. As discussed in PR feedback, provisioning is core to azd and we should maintain a single canonical config type per concern rather than duplicating structs across packages. Co-authored-by: kristenwomack <5034778+kristenwomack@users.noreply.github.com>
Reverted all changes in commit 8ea3d99. The duplicate InfraConfig type has been removed and ProjectConfig/ServiceConfig now use provisioning.Options directly as before. I agree that maintaining a single canonical config type per concern is the right approach. |
This PR reverts the changes that attempted to decouple
ProjectConfigfromprovisioning.Optionsby introducing a duplicateInfraConfigtype.Reason for Revert
Based on PR feedback, the original approach was not aligned with the project's architecture principles:
Changes Reverted
InfraConfigtype and conversion functions (infra_config.go,infra_config_test.go)ProjectConfig.Infrato useprovisioning.OptionsdirectlyServiceConfig.Infrato useprovisioning.Optionsdirectlyprovisioning.Optionswithout conversion layerCurrent State
The codebase now maintains the original architecture where:
ProjectConfigandServiceConfigdirectly useprovisioning.OptionsOriginal prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.