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 data_source_dashboards #378

Merged
merged 28 commits into from Feb 21, 2022

Conversation

justinTM
Copy link
Contributor

@justinTM justinTM commented Feb 6, 2022

builds haven't been initiating so trying new fresh branch

@justinTM justinTM requested a review from a team as a code owner February 6, 2022 00:05
@justinTM
Copy link
Contributor Author

justinTM commented Feb 6, 2022

@inkel builds don't appear to be working

@inkel
Copy link
Contributor

inkel commented Feb 7, 2022

Hi, @justinTM, thanks again for your contributions. I've got a question: how's this PR different from #378? Both have the same title and seem to be adding the same kind of data source.

Also, I'm not so sure about this one. We already have a grafana_dashboard data source introduced in #359, I think we should in any case extend that one to allow for returning more than one result instead.

Let's stop development on this one temporarily while we discuss it with the rest of the maintainers.

Thanks!

@justinTM justinTM closed this Feb 7, 2022
@justinTM justinTM reopened this Feb 7, 2022
@justinTM
Copy link
Contributor Author

justinTM commented Feb 8, 2022

hey @inkel , they are virtually the same PRs; i was trying to see if i could trigger the CI build using a different branch.

i would love to pause development honestly, but unfortunately there is a vulnerability in the way grafana provisioning (with terraform) mixes with user-created dashboards: dashboards in provisioned folders will be lost forever. the organization i'm working with has been experiencing this issue with increasing frequency lately.

from a development standpoint, i'm testing cURL requests directly to the API with good (sometimes unexpected) results. i would just use shell commands in the project but keeping everything maintained by terraform is preferable.

data_source_dashboard.go could be made responsible for getting all dashboards, but currently that would require refactoring the required inputs (for id, uid, etc.). the way the aws provider seems to do it -- similar to Trott's probes datasource -- is to make them separate:

if i can refactor the dashboard folder search function in the client and use it successfully, i think this PR will be ready for review.

@justinTM
Copy link
Contributor Author

justinTM commented Feb 9, 2022

@inkel wow what a relief. I finally figured it out. There's a bug in the testing framework from Hashicorp: hashicorp/terraform-plugin-sdk#885

@justinTM
Copy link
Contributor Author

hey @inkel our team needs this ASAP if I could bother you for a review.

@julienduchesne
Copy link
Member

hey @inkel our team needs this ASAP if I could bother you for a review.

Hey @justinTM. Unfortunately, the provider is only one of our responsibilities within Grafana Labs. We actively watch PRs with passing checks and we'll eventually get to yours when we get the time. Thanks for your patience!

@justinTM
Copy link
Contributor Author

the tests are passing

@justinTM
Copy link
Contributor Author

@julienduchesne do you know of a way to utilize a forked repo without publishing to the Hashicorp Registry?

@julienduchesne
Copy link
Member

@julienduchesne do you know of a way to utilize a forked repo without publishing to the Hashicorp Registry?

You can write custom versions of a provider (or completely create a new one) locally

Here's what I use from my repo:

go build && cp ./terraform-provider-grafana ~/.terraform.d/plugins/registry.terraform.io/grafana/grafana/999.999.999/darwin_amd64/terraform-provider-grafana

Then my Grafana config looks like this:

terraform {
  required_providers {
    grafana = {
      source = "grafana/grafana"
    }
  }
}

provider "grafana" {
  url  = "http://localhost:3000"
  auth = "admin:admin"
}

@inkel
Copy link
Contributor

inkel commented Feb 10, 2022

As an alternative, if you use terraform CLI tool you can override the provider installation via a configuration file.

Copy link
Contributor

@inkel inkel left a comment

Choose a reason for hiding this comment

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

Upon first review looks quite good, however, we cannot accept the replace directive in go.mod.

go.mod Outdated Show resolved Hide resolved
@justinTM
Copy link
Contributor Author

@inkel i know, it's just to get passing tests until client PR is merged

@justinTM
Copy link
Contributor Author

must be merged together with grafana/grafana-api-golang-client#59

i'll remove the replace directive in anticipation

Copy link
Member

@julienduchesne julienduchesne left a comment

Choose a reason for hiding this comment

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

LGTM modulo rebasing on master + the two comments I added

grafana/data_source_dashboards.go Outdated Show resolved Hide resolved
grafana/data_source_dashboards.go Outdated Show resolved Hide resolved
@justinTM
Copy link
Contributor Author

Copy link
Contributor

@inkel inkel left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for your work and patience!

sort.Strings(paramsList)
hashIn := strings.Join(paramsList, "")
hashOut.Write([]byte(hashIn))
return fmt.Sprintf("%x", hashOut.Sum(nil))[0:23]
Copy link
Contributor

Choose a reason for hiding this comment

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

Just out of curiosity, any reason why returning just 24 characters?

grafana/data_source_dashboards.go Outdated Show resolved Hide resolved
Co-authored-by: Leandro López <inkel.ar@gmail.com>
@julienduchesne julienduchesne merged commit 2542182 into grafana:master Feb 21, 2022
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

3 participants