fix: panic using project_verisoning_strategy without donorpackage #859
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
The Template and DonorPackage are mutually exclusive options, but this isn't reflected in the schema or our internal translation logic between TF State and API Resources. Update the mapping logic to account for DonorPackage sometimes being null. Fixes #855
|
[sc-102624] |
The schema validation on octopusdeploy_project_verisoning_strategy doesn't match what Octopus Server actually requires. The template and donor_package attributes should be mutually exclusive; donor_package shouldn't always be required. This change: - Adds an EntitySchemaWithResourceValidators interface - Implements this for Project Version Strategy - Makes schema attributes template, donor_package, donor_package_step_id all optional - Makes child attributes of donor_package required, if it's being specified - Validates that either one of template OR donor_package is used - Validates that donor_package and donor_package_step_id must occur together - Makes space_id optional, allowing fallback to the provider configured SpaceId like other resources
domenicsim1
approved these changes
Feb 6, 2025
| if !(state.DonorPackageStepID.IsNull()) { | ||
| projectVersioningStrategy.DonorPackageStepID = state.DonorPackageStepID.ValueStringPointer() | ||
| } | ||
| if !(state.DonorPackage == nil) { |
There was a problem hiding this comment.
I am guessing you do this to match the above line over != nil?
There was a problem hiding this comment.
Yeah, consistency of... polarity?... of the check, so that it's easy to scan down and understand. I would've preferred an IsNull() method, but it doesn't exist on the DonorPackage struct versus the String above.
Comment on lines
+58
to
+73
| func (p ProjectVersioningStrategySchema) GetResourceConfigValidators() []resource.ConfigValidator { | ||
| return []resource.ConfigValidator{ | ||
| resourcevalidator.RequiredTogether( | ||
| path.MatchRoot("donor_package"), | ||
| path.MatchRoot("donor_package_step_id"), | ||
| ), | ||
| resourcevalidator.Conflicting( | ||
| path.MatchRoot("template"), | ||
| path.MatchRoot("donor_package"), | ||
| ), | ||
| resourcevalidator.Conflicting( | ||
| path.MatchRoot("template"), | ||
| path.MatchRoot("donor_package_step_id"), | ||
| ), | ||
| } | ||
| } |
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.
The Template and DonorPackage on a Project Versioning Strategy are mutually exclusive options, but this isn't reflected in the schema or our internal translation logic between TF State and API Resources.
Our mapping logic between the TF State and the API Resource didn't account for this, which causes #855 - the provider works fine if the
donor_packageapproach is used, but crashes if thetemplateapproach is used.I took the opportunity to also fix up the schema, validation, docs and examples to better represent the actual valid inputs from the Octopus Server perspective:
donor_packagein the schema from Required to Optionaldonor_packageto be required, whendonor_packageis presentdonor_packageanddonor_package_step_idmust be present togethertemplateordonor_packagecan be usedFixes #855
Note: These updates are not a breaking change, because even though it was previously possible from a TF perspective to specify both
donor_packageandtemplatein a config, Octopus Server API has a server-side check that prevents such a strategy from being applied, and fails theterraform apply, so it's not possible for any customers to have a config that will break with this change. If that server-side check hadn't existed, we would have had to mark this as breaking due to the stricter requirements on the mutual-exclusivity.