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

add workspaces data source (v1beta1 list workspaces equivalent) #6

Merged
merged 7 commits into from
Apr 8, 2024

Conversation

vandyliu
Copy link
Collaborator

@vandyliu vandyliu commented Apr 4, 2024

Description

  • Add the workspaces data source to the terraform provider
  • Make some repo structural changes: update README, move some files around, change some import names

Describe the purpose of this pull request.

🎟 Issue(s)

(https://github.com/astronomer/astro/issues/19655)

🧪 Functional Testing

main.tf file:

data "astronomer_workspaces" "list_workspaces" {
}

output "list_workspaces" {
  value = data.astronomer_workspaces.list_workspaces
}

output:

list_workspaces = {
  "names" = tolist(null) /* of string */
  "workspace_ids" = tolist(null) /* of string */
  "workspaces" = tolist([
    {
      "cicd_enforced_default" = false
      "created_at" = "2024-04-05 21:41:04.673 +0000 UTC"
      "created_by" = {
        "api_token_name" = "tf"
        "avatar_url" = tostring(null)
        "full_name" = tostring(null)
        "id" = "clthx7ect001l01i2bns855s2"
        "subject_type" = "SERVICEKEY"
        "username" = tostring(null)
      }
      "description" = "hi kushal"
      "id" = "clun6x735000701m5g5grj3sj"
      "name" = "tf-workspace-12"
      "organization_name" = "hosted123"
      "updated_at" = "2024-04-05 21:41:04.673 +0000 UTC"
      "updated_by" = {
        "api_token_name" = "tf"
        "avatar_url" = tostring(null)
        "full_name" = tostring(null)
        "id" = "clthx7ect001l01i2bns855s2"
        "subject_type" = "SERVICEKEY"
        "username" = tostring(null)
      }
    },
}]

List the functional testing steps to confirm this feature or fix.

📸 Screenshots

Screenshot 2024-04-03 at 7 48 25 PM

Add screenshots to illustrate the validity of these changes.

📋 Checklist

  • Rebased from the main (or release if patching) branch (before testing)
  • Ran make test before taking out of draft
  • Ran make fmt before taking out of draft
  • Added/updated applicable tests
  • Tested against Astro-API (if necessary).
  • Communicated to/tagged owners of respective clients potentially impacted by these changes.
  • Updated any related documentation

@vandyliu vandyliu changed the title add workspaces data source add (list) workspaces data source Apr 5, 2024
@vandyliu vandyliu changed the title add (list) workspaces data source add workspaces data source (v1beta1 list workspaces equivalent) Apr 5, 2024
@vandyliu vandyliu marked this pull request as ready for review April 5, 2024 21:49
@vandyliu vandyliu requested review from sunkickr and a team as code owners April 5, 2024 21:49
Comment on lines +105 to +107
if resp.Diagnostics.HasError() {
return
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

does this print the error msg out too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep, whatever is in resp.Diagnostics gets printed out at the end

sometimes, you'll see that i add my own error thats because the error didnt come from terraform but from somewhere else so terraform doesnt know an error occured, eg. if we called the api and there was an error, so in that case I add our own error to resp.Diagnostics

@@ -107,7 +107,11 @@ func (d *workspaceDataSource) Read(
}

// Populate the model with the response data
data.ReadFromResponse(ctx, workspace.JSON200)
diags := data.ReadFromResponse(ctx, workspace.JSON200)
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is workspace.JSON200? and why is it called JSON200 (is it supposed to represent a 200 resp code?)

notice this being referenced and used a lot

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

workspace is the response object from the API client, workspace.JSON200 gives us the platformv1beta1.Workspace object

we use a different client here than we do for the component tests, which is why there are these syntax differences

"id": datasource.StringAttribute{
func WorkspaceDataSourceSchemaAttributes() map[string]datasourceSchema.Attribute {
return map[string]datasourceSchema.Attribute{
"id": datasourceSchema.StringAttribute{
MarkdownDescription: "Workspace identifier",
Copy link
Collaborator

Choose a reason for hiding this comment

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

general q - with the MarkdownDescription for all the Schema Attributes, how does this actually look, like do you have an example? will this operate similar to doc strings in our models?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

its in docs/data-sources/workspaces.md

Makefile Outdated Show resolved Hide resolved
README.md Outdated
@@ -35,6 +35,7 @@ Then commit the changes to `go.mod` and `go.sum`.
## Using the provider

```shell
export ASTRI_API_TOKEN=<token>

Choose a reason for hiding this comment

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

Suggested change
export ASTRI_API_TOKEN=<token>
export ASTRO_API_TOKEN=<token>

Choose a reason for hiding this comment

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

Should we add a note and link to the doc on how to create this token?

https://docs.astronomer.io/astro/automation-authentication#step-1-create-an-api-token

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

definitely


### Optional

- `names` (List of String)

Choose a reason for hiding this comment

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

Is this supposed to be list of workspace names?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep that you can query if you want

func WorkspaceDataSourceSchemaAttributes() map[string]datasource.Attribute {
return map[string]datasource.Attribute{
"id": datasource.StringAttribute{
func WorkspaceDataSourceSchemaAttributes() map[string]datasourceSchema.Attribute {

Choose a reason for hiding this comment

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

Can we add some tests for both get workspace and list workspaces?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

terraform unit tests aren't really a thing with the framework - they just do acceptance integration tests
those will be set up in the near future

Copy link

@kushalmalani kushalmalani left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the comments.

@vandyliu vandyliu merged commit 5c6a204 into main Apr 8, 2024
4 checks passed
@vandyliu vandyliu deleted the workspaces-datasource branch April 8, 2024 18:02
terraform {
required_providers {
astronomer = {
source = "registry.terraform.io/astronomer/astronomer"
Copy link

Choose a reason for hiding this comment

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

should this be astronomer/astro since the provider is for astro the product?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure we can change it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

what do you think @sunkickr ?

Choose a reason for hiding this comment

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

astronomer/astro makes more sense to me as well

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.

4 participants