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

[Embed API] Ruby Client Library and Tests #73

Merged
merged 5 commits into from
Jul 29, 2015
Merged

Conversation

sup
Copy link
Contributor

@sup sup commented Jul 16, 2015

Ruby client library to make calls to the new Embed API. Added function calls and tests.

@sup sup closed this Jul 16, 2015
@sup sup reopened this Jul 16, 2015
@sup
Copy link
Contributor Author

sup commented Jul 16, 2015

nondeterministic failures are the best kind of failures /s

@sup sup added 3 - Done and removed 3 - Done labels Jul 17, 2015
@embed_svc.get_embed(embed_id, size, legend, template_vars)
end

def create_embed(graph_json, timeframe="1_hour", size="medium", legend="no", title="Embed created through API")
Copy link
Member

Choose a reason for hiding this comment

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

Is the title parameter mandatory ?

From documentation page, it looks like it's automatically set when no value is submitted, i.e.:

curl -POST \
    -d 'graph_json={"requests":[{"q":"avg:system.load.1{*}"}],"viz":"timeseries","events":[]}' \
    -d "timeframe=1_hour" \
    -d "size=medium" \
    -d "legend=yes" \
    "https://app.datadoghq.com/api/v1/graph/embed?api_key=${api_key}&application_key=${app_key}"

leads to

{   
  "embed_id": "5f585b01c81b12ecdf5f40df0382738d0919170639985d3df5e2fc4232865b0c", 
  "template_variables": [], 
  "html": "<iframe src=\"https://app.datadoghq.com/graph/embed?token=5f585b01c81b12ecdf5f40df0382738d0919170639985d3df5e2fc4232865b0c&height=300&width=600&legend=true\" width=\"600\" height=\"300\" frameBorder=\"0\"></iframe>", 
  "graph_title": "Embed created through API", 
  "revoked": false, 
  "dash_url": null, 
  "shared_by": 3658, 
  "dash_name": null

(note the graph_title field value)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yannmh It isn't required. Did I make title required in the Ruby code? I'm not super familiar with Ruby so I assumed the default parameters work the same way as they do in Python

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well titles are required in the sense that the embed has to have a title at least 1 character in length and the default title is "Embed created through API"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess the default params can be nil

@yannmh
Copy link
Member

yannmh commented Jul 28, 2015

It looks great overall.

However, I think we should make all optional parameters part of a kwarg, like we do for most of our API features (unfortunately not all 👎 ).
i.e having

dog.create_embed(graph_json, :timeframe => timeframe, :size => size, :legend => legend ...)

rather than

dog.create_embed(graph_json, timeframe, size, legend, title)

@yannmh yannmh self-assigned this Jul 28, 2015
@sup
Copy link
Contributor Author

sup commented Jul 28, 2015

Changed parameters to use a dictionary/hash kwargs

@yannmh
Copy link
Member

yannmh commented Jul 29, 2015

Thanks for the update @charleslai.

I added two nitpicks. Could you also update the documentation to reflect those changes please ?

# Verify that the get requests are good
assert embed_without_var["html"] == create_result["html"]
assert embed_with_var["html"] != embed_without_var["html"]
end
Copy link
Member

Choose a reason for hiding this comment

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

Can you check that the values returned match the description ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I sort of do a check by checking if the html after using get is different from the html that was originally created but I guess I could try parsing the html string

@yannmh
Copy link
Member

yannmh commented Jul 29, 2015

Great work. Thanks for addressing all the comments.

I am waiting for the tests to complete and merging.

yannmh added a commit that referenced this pull request Jul 29, 2015
[Embed API] Ruby Client Library and Tests
@yannmh yannmh merged commit 69d58c1 into master Jul 29, 2015
@yannmh yannmh deleted the charles/embed_api_rb branch July 29, 2015 16:42
yannmh added a commit that referenced this pull request Jul 29, 2015
* [FEATURE] Embeddable graphs API. See [#73][]

[#73]: #73
yannmh added a commit that referenced this pull request Jul 29, 2015
* [FEATURE] Embeddable graphs API. See [#73][]

[#73]: #73
[skip ci]
@yannmh yannmh mentioned this pull request Jul 29, 2015
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.

2 participants