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

WIP: Implement looker_project and more #15

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

lukas-at-harren
Copy link

@lukas-at-harren lukas-at-harren commented Nov 24, 2021

I added the following resources:

  • looker_project
  • looker_project_git_deploy_key
  • looker_project_git_repository

Those allow to setup Looker projects and git configuration.
In order for them to work properly, I had to fix the looker SDK (See also: looker-open-source/sdk-codegen#912).

Please only merge after:

  • Looker SDK has merged my fix upstream
  • This provider is updated to the latest SDK
  • The additions have been reviewed and tested by more people than me

––

Of course adding projects via Terraform is not without issues.
Besides the bug in the golang looker API client, there are some issues with the Looker API itself.

One is, that the API does not support deleting projects.
=> I solved this by issuing a warning to the user of the resource.

Another is, that the API client session must be forced into "dev"-mode in order to create projects.
=> That I am currently still testing.
=> One suggestion I have, if this turns out to be a major hassle, is to allow for a "workspace_id" configuration at the provider level. That way, the user could configure two looker providers, one per workspace_id (production, dev) and this problem would also be solved.

Adding a git-repository to a Looker project is a major hassle.
=> I attempt to solve this by having separate resources dealing with the git deploy key and the git repository configuration of a project. This way, a user of the resources can create a proper dependency graph creating a deploy key and assigning the deploy key to a Github project via the terraform github provider.

@@ -326,7 +326,7 @@ func expandWriteDBConnection(d *schema.ResourceData) (*apiclient.WriteDBConnecti

// optional values
if v, ok := d.GetOk("port"); ok {
port := int64(v.(int))
Copy link
Author

Choose a reason for hiding this comment

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

The latest version of the looker sdk has this field noted as string.

@hirosassa
Copy link
Owner

@lukas-at-harren Thank you for your contribution! I'll take a look tonight!

@lukas-at-harren
Copy link
Author

I will check test failures this week.

@hirosassa
Copy link
Owner

@lukas-at-harren Hi, lukas! What's the status of this and related PR?

@lukas-at-harren
Copy link
Author

lukas-at-harren commented Mar 9, 2022

@hirosassa hi, sorry for the massive delay in everything. But the people over at the Looker-SDK seem to have no time/attention for the PR I posted there. I bumped them in a comment, and hope to get things moving.

From my side, this is still active, and I hope that this contribution makes it through.
Unfortunate this is dependent on an implementation detail of the Looker SDK. I see no way around this, because the additions of my contribution require the SSH public key to be readable from the Looker API. Exactly that endpoint is broken with the current implementation of the Looker SDK (it simply assumes ALL responses are JSON... and therefore fails parsing them).

@lukas-at-harren
Copy link
Author

@hirosassa there has been some movement upstream, my change/fix was not merged but there is now looker-open-source/sdk-codegen#1021 that should close this bug.

@hirosassa
Copy link
Owner

@lukas-at-harren Hi! Could you update this PR? sdk-codegen is updated!

@lukas-at-harren
Copy link
Author

@hirosassa hi, I finally got to rebase the PR onto your master. It seems to work fine (on our end).
I am just checking the lint.

Copy link
Owner

@hirosassa hirosassa 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 your contribution! I added a few comments. Please check!

}
return diag.FromErr(err)
}
d.Set("git_deploy_key", gitDeployKey)
Copy link
Owner

Choose a reason for hiding this comment

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

should check err like below:

Suggested change
d.Set("git_deploy_key", gitDeployKey)
if err = d.Set("git_deploy_key", gitDeployKey); err != nil {
return diag.FromErr(err)
}

if err != nil {
return diag.FromErr(err)
}
d.Set("git_deploy_key", gitDeployKey)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
d.Set("git_deploy_key", gitDeployKey)
if err = d.Set("git_deploy_key", gitDeployKey); err != nil {
return diag.FromErr(err)
}

if err != nil {
return diag.FromErr(err)
}
d.Set("git_service_name", project.GitServiceName)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
d.Set("git_service_name", project.GitServiceName)
if err = d.Set("git_service_name", project.GitServiceName); err != nil {
return diag.FromErr(err)
}

return diag.FromErr(err)
}
d.Set("git_service_name", project.GitServiceName)
d.Set("git_remote_url", project.GitRemoteUrl)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
d.Set("git_remote_url", project.GitRemoteUrl)
if err = d.Set("git_remote_url", project.GitRemoteUrl); err != nil {
return diag.FromErr(err)
}

writeProject := apiclient.WriteProject{
Name: &projectName,
}
project, err := client.CreateProject(writeProject, nil)
Copy link
Owner

Choose a reason for hiding this comment

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

Cloud you add [DEBUG] log before api call in Create, Read, Update, and Delete?

For example,
https://github.com/hirosassa/terraform-provider-looker/blob/master/pkg/looker/resource_user_attribute.go#L75

@hirosassa
Copy link
Owner

@lukas-at-harren Hi, what's the status?

@Nabil-Lahssini
Copy link

Hi guys, just to let you know that a new provider has been released with all the functionalities, project creation etc. It's open source. Feel free to use it and contribute
https://github.com/devoteamgcloud/terraform-provider-looker

@vmichel95
Copy link

Hi guys, just to let you know that a new provider has been released with all the functionalities, project creation etc. It's open source. Feel free to use it and contribute https://github.com/devoteamgcloud/terraform-provider-looker

@Nabil-Lahssini can you add support for looker_permission_set resources

@Nabil-Lahssini
Copy link

@Nabil-Lahssini can you add support for looker_permission_set resources

@vmichel95 Hi, thank you for your response, support for looker_permission_set was just added today in version 0.2.0. Feel free to use it and give some feedback in the discussions section :) . You can also discuss other functionalities you want to see in future versions.

* upstream/master:
  Allow updating connection certificate (hirosassa#40)
  Fix hidden value domain whitelist (hirosassa#38)
  Update release.yml
  Update workflow (hirosassa#37)
  Fix user attribute user value delete (hirosassa#36)
  Support HiddenValueDomainWhitelist (hirosassa#35)
  Support IsDisabled (hirosassa#34)
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.

None yet

5 participants