-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add deployment resource #9
Conversation
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.
moved into data_worksapces_test.go since code is similar and we can check both in same test using same workspace resource
"github.com/stretchr/testify/assert" | ||
) | ||
|
||
func TestUnit_Validators_ListIsCuids(t *testing.T) { |
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.
don't need this anymore since there's actually a list validator we can use
if request.ConfigValue.IsNull() || request.ConfigValue.IsUnknown() { | ||
return | ||
} | ||
|
||
value := request.ConfigValue.String() | ||
value := request.ConfigValue.ValueString() |
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.
.ValueString()
gives us the actual string str
.String()
gives us "str"
<!-- schema generated by tfplugindocs --> | ||
## Schema | ||
|
||
### Required |
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.
are we just writing these manually every time?
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.
its generated by tfplugindocs (line 1) made by make build
defined by our schema
@@ -0,0 +1,190 @@ | |||
--- |
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.
general question, is it conventional to have separate data sources to each of the oneof types?
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.
i cant find many examples since it seems that most of the time the API matches the terraform provider in terms of resources.
Let me take a stab at making it all the same (ie. deployment_resource) to see what complexities it may bring up
- `webserver_airflow_api_url` (String) Deployment webserver Airflow API URL | ||
- `webserver_ingress_hostname` (String) Deployment webserver ingress hostname | ||
- `webserver_url` (String) Deployment webserver URL | ||
- `workload_identity` (String) Deployment workload identity. This value can be changed via the Astro API if applicable. |
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.
Should this be required? I believe a user can update this via cli
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.
its a weird one because it is auto-generated if no input
furthermore it can only be done for certain deployments like aws hybrid deployments and gcp hosted deployments so we cant have it be required nor optional
if we set it to optional and it is blank then terraform will complain because the astro api sets it but terraform expects it to be blank
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.
What would the user do if they use aws hybrid dpeloyments and want to update workload_identity. Dont want to block this PR because of this, but something for us to think about
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.
they can update it via API and the value will be left alone if terraform ever does any updates
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.
thanks for the walk through yesterday, i think we can merge this and start internal testing
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.
Looks good. Thanks
Description
🎟 Issue(s)
#10
🧪 Functional Testing
Hosted deployment config
Hybrid deployment config
📸 Screenshots
📋 Checklist
make test
before taking out of draftmake fmt
before taking out of draft