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

Switch from call_id to task_id for consistency #553

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

Conversation

denismakogon
Copy link
Contributor

@denismakogon denismakogon commented Mar 1, 2017

As part of async execution HTTP API response IronFunctions returns call_id. But it's actually referring to task ID (from models.Task). In order to keep consistency between tasks objects (that are available via /tasks HTTP API) that are containing task_id and no call_id it is necessary to rename call_id to task_id.

This patch renames call_id that comes as part of API response on async execution request to task_id for API consistency purposes.

HTTP API impact: call_id renamed to task_id

Partially addresses: #415

@treeder
Copy link
Contributor

treeder commented Mar 2, 2017

Is this only for async functions? We named them call_id in reference to a "functions calls", not sure we should have task_id and call_id.

@denismakogon
Copy link
Contributor Author

@treeder the thing is that Async execution returns call_id, but I need to query /tasks to get info on it. So, for implementing /tasks/{task_id} we need to clarify what we return to a users, would it be call_id or taks_id that can be tracked via /tasks

@denismakogon
Copy link
Contributor Author

@seiflotfy @pedronasser @treeder Guys, can we make certain decision on this one?

@seiflotfy
Copy link
Contributor

So I am pretty ok with that change, we are not 1.0 and I like the changes to the docs.
I'd like to have a +1 from @pedronasser @treeder @ccirello
Then lets push it through. Also can you make sure you add a note in the changelogs?

Copy link
Contributor

@seiflotfy seiflotfy left a comment

Choose a reason for hiding this comment

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

LGTM

@ucirello
Copy link
Contributor

A good merge message covering on the reason and consequences of this change is a must. Regarding everything else, LGTM.

@treeder
Copy link
Contributor

treeder commented Mar 22, 2017

The main thing that bugs me about this is having different names for essentially the same thing. We should choose one or the other across the board.

@denismakogon
Copy link
Contributor Author

@treeder isn't task_id fits better here rather than call_id?

@CLAassistant
Copy link

CLAassistant commented Apr 7, 2017

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@ucirello ucirello left a comment

Choose a reason for hiding this comment

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

I am just trying to decouple my account from this.

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.

6 participants