-
Notifications
You must be signed in to change notification settings - Fork 105
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
Remove address
param from grafana_organization
#284
Conversation
end | ||
|
||
def organization | ||
@organization ||= organizations.find { |x| x[:name] == resource[:name] } |
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.
Fetching details of all orgs for every grafana_organization
resource only to throw away all but the one you care about here was pretty wasteful in terms of API calls.
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.
API calls needed for each organizations grows exponentially in fact.
end | ||
|
||
def id=(value) | ||
resource[:id] = value |
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.
If this line of code was ever hit, resource[:id]
would already equal value
, and you can't update the database id of a grafana resource anyway.
end | ||
|
||
def address | ||
organization[:json_data] |
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 was json_data
??
|
||
response = send_request('POST', format('%s/orgs', resource[:grafana_api_path]), data) if organization.nil? | ||
|
||
raise format('Failed to create save %s (HTTP response: %s/%s)', resource[:name], response.code, response.body) if response.code != '200' |
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.
if organization.nil?
wasn't true (above), this line would explode calling code
on response
(which would be nil
).
de38763
to
55fbb2d
Compare
Organization Info (ie. its address) was removed from the Grafana UI in grafana/grafana@21a7f57 According to https://grafana.com/docs/grafana/v8.4/http_api/org/#update-organization the API was not fully implemented, so I'm unconvinced this ever worked. The property `id` is also removed as it's internal to grafana and not something you could ever update. Updating a `grafana_organization` never worked as `save_organization` only implemented POSTs, (and would actually explode if you tried to update `address`)
55fbb2d
to
0d2a1e8
Compare
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.
while the change looks good I am not sure about the label. I think it never worked properly so we could call it bugfix, but it coul also be a breaking change
Yeah, not sure either. Perhaps before an actual release, they'll be some actual breaking change and then we may as well call this one breaking too. |
Organization Info (ie. its address) was removed from the Grafana UI in
grafana/grafana@21a7f57
According to
https://grafana.com/docs/grafana/v8.4/http_api/org/#update-organization
the API was not fully implemented, so I'm unconvinced this ever worked.
The property
id
is also removed as it's internal to grafana and notsomething you could ever update.
Updating a
grafana_organization
never worked assave_organization
only implemented POSTs, (and would actually explode if you tried to
update
address
)