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

fix: fix datasource result blocks #763

Merged
merged 14 commits into from
Aug 22, 2024
Merged

fix: fix datasource result blocks #763

merged 14 commits into from
Aug 22, 2024

Conversation

benPearce1
Copy link
Collaborator

@benPearce1 benPearce1 commented Aug 22, 2024

related to #762

Copy link
Collaborator

@IsaacCalligeros95 IsaacCalligeros95 left a comment

Choose a reason for hiding this comment

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

Project Groups still needs updating.

Variables => Scope still has a List Nested Block, I think this might be intentional and part of the actual config but taking a look would be good.

@benPearce1
Copy link
Collaborator Author

benPearce1 commented Aug 22, 2024

Variables => Scope still has a List Nested Block, I think this might be intentional and part of the actual config but taking a look would be good.

@IsaacCalligeros95 I think when it comes to the datasource result set attributes, it probably doesn't matter. I think the resource schema types matter more

@@ -37,24 +36,11 @@ data "octopusdeploy_variables" "example" {
- `is_editable` (Boolean) Indicates whether or not this variable is considered editable.
- `is_sensitive` (Boolean) Indicates whether or not this resource is considered sensitive and should be kept secret.
- `prompt` (Attributes List) (see [below for nested schema](#nestedatt--prompt))
- `scope` (Attributes List) As variable names can appear more than once under different scopes, a VariableScope must also be provided (see [below for nested schema](#nestedatt--scope))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking at the old schema docs it looks like the scope should be Required, sorry I think I mislead you on the last review.

https://github.com/OctopusDeployLabs/terraform-provider-octopusdeploy/blob/isaac/v0.22.1-dsfix-copy/docs/data-sources/variables.md

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the scope is a nested property of the variable, within a datasource the results are all readonly, I think this feels right


- `description` (String)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think these should still be Read only.

Copy link
Collaborator

@IsaacCalligeros95 IsaacCalligeros95 left a comment

Choose a reason for hiding this comment

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

A couple of minor comments.

Copy link
Collaborator

@IsaacCalligeros95 IsaacCalligeros95 left a comment

Choose a reason for hiding this comment

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

LGTM

@benPearce1 benPearce1 merged commit 7217313 into main Aug 22, 2024
22 checks passed
@benPearce1 benPearce1 deleted the bp/framework-ds-fix branch August 22, 2024 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants