Skip to content
This repository has been archived by the owner on Jan 15, 2024. It is now read-only.

Add organization ID to client connection using header #9

Merged
merged 4 commits into from
Oct 28, 2020
Merged

Add organization ID to client connection using header #9

merged 4 commits into from
Oct 28, 2020

Conversation

medains
Copy link
Contributor

@medains medains commented Oct 16, 2020

This seems like the right place to do so - the resource API calls that require an organization ID that are not-quite RESTful also would need authentication with a user that is an administrator for the organization. So putting it in at this level works out, and also avoids adding orgID parameters all over the place.

Co-authored-by: hansnqyr [email protected]

This change was previously submitted to nytm/go-grafana-api#62
It is a prerequisite for this PR: grafana/terraform-provider-grafana#110

@aknuds1 aknuds1 changed the title Add organization ID to client connection using header. Add organization ID to client connection using header Oct 16, 2020
@aknuds1
Copy link
Contributor

aknuds1 commented Oct 16, 2020

Having a look.

Copy link
Contributor

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

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

There are some changes I'd like to see, but first checking with my colleagues about the proposal.

Copy link
Contributor

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

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

Just some quick feedback for now, to collect feedback until I have the time for a real review:

  • Org ID should not be provided if authing with an API key, because an API key is for a single org and role, so only applicable with basic auth
  • Making org ID mandatory for creating a client is a bad idea
  • Allowing the org ID header to be set when using basic auth makes sense

@aknuds1 aknuds1 mentioned this pull request Oct 17, 2020
@aknuds1
Copy link
Contributor

aknuds1 commented Oct 19, 2020

Can you please merge master into the PR, and fix conflicts? I changed the signature of Client.New, so you can add the organization ID to the Config type parameter.

@medains
Copy link
Contributor Author

medains commented Oct 19, 2020

I shall do - those changes for the config make adding optional orgid a much cleaner change - thanks. Will update in the next day or so.

This seems like the right place to do so - the resource API calls that
require an organization ID that are not-quite RESTful also would need
authentication with a user that is an administrator for the
organization.  So putting it in at this level works out, and also avoids
adding orgID parameters all over the place.

Co-authored-by: hansnqyr <[email protected]>
@medains
Copy link
Contributor Author

medains commented Oct 21, 2020

I've rebased and the change is now much simpler - thanks.

@aknuds1
Copy link
Contributor

aknuds1 commented Oct 21, 2020

Thanks, I'll have a look when I get the time.

@aknuds1
Copy link
Contributor

aknuds1 commented Oct 26, 2020

Having a look again.

Copy link
Contributor

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

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

Looks much better now, thanks! I will try to get feedback from @marefr this week to make sure this change makes sense.

Requesting changes to signal it still needs to be decided on.

client_test.go Outdated Show resolved Hide resolved
@aknuds1 aknuds1 requested a review from marefr October 27, 2020 16:21
Copy link
Contributor

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

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

LGTM, but @marefr please review given your knowledge of the functionality.

Copy link

@marefr marefr left a comment

Choose a reason for hiding this comment

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

Minor change. See comment. Otherwise looks good I think 👍

client.go Outdated Show resolved Hide resolved
@aknuds1 aknuds1 requested a review from marefr October 28, 2020 09:47
Copy link

@marefr marefr left a comment

Choose a reason for hiding this comment

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

LGTM

@aknuds1 aknuds1 merged commit dd7fea8 into grafana:master Oct 28, 2020
@aknuds1
Copy link
Contributor

aknuds1 commented Oct 28, 2020

Thanks for the contribution!

medains added a commit to medains/terraform-provider-grafana that referenced this pull request Oct 28, 2020
Use the change in grafana/grafana-api-golang-client#9 to
apply an organization id to the provider.

Usage might look like this:

```hcl
provider "grafana" {
  url = "someurl"
  auth = "adminuser:somepass"
}

resource "grafana_organization" "neworg" {
  name = "neworg"
  admin = "neworgadmin"
}

provider "grafana" {
  alias = "neworg"
  url = "someurl"
  auth = "neworgadmin:somepass"
  org_id = grafana_organization.neworg.org_id
}

resource "grafana_data_source" "neworg_graphite" {
  provider = grafana.neworg
  type = "graphite"
  name = "neworg-graphite"
}
```

Co-authored-by: hansnqyr <[email protected]>
medains added a commit to medains/terraform-provider-grafana that referenced this pull request Oct 28, 2020
Use the change in grafana/grafana-api-golang-client#9 to
apply an organization id to the provider.

Usage might look like this:

```hcl
provider "grafana" {
  url = "someurl"
  auth = "adminuser:somepass"
}

resource "grafana_organization" "neworg" {
  name = "neworg"
  admin = "neworgadmin"
}

provider "grafana" {
  alias = "neworg"
  url = "someurl"
  auth = "neworgadmin:somepass"
  org_id = grafana_organization.neworg.org_id
}

resource "grafana_data_source" "neworg_graphite" {
  provider = grafana.neworg
  type = "graphite"
  name = "neworg-graphite"
}
```

Co-authored-by: hansnqyr <[email protected]>
medains added a commit to medains/terraform-provider-grafana that referenced this pull request Oct 28, 2020
Use the change in grafana/grafana-api-golang-client#9 to
apply an organization id to the provider.

Usage might look like this:

```hcl
provider "grafana" {
  url = "someurl"
  auth = "adminuser:somepass"
}

resource "grafana_organization" "neworg" {
  name = "neworg"
  admin = "neworgadmin"
}

provider "grafana" {
  alias = "neworg"
  url = "someurl"
  auth = "neworgadmin:somepass"
  org_id = grafana_organization.neworg.org_id
}

resource "grafana_data_source" "neworg_graphite" {
  provider = grafana.neworg
  type = "graphite"
  name = "neworg-graphite"
}
```

Co-authored-by: hansnqyr <[email protected]>
client.go Show resolved Hide resolved
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants