Skip to content

GET entrypoint of a workflow from st2client #4791

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

Merged
merged 6 commits into from
Oct 10, 2019

Conversation

trstruth
Copy link
Member

@trstruth trstruth commented Sep 17, 2019

This PR implements a method in the st2client module which fetches the entrypoint of a given action ref/id. This is essentially a wrapper of this endpoint.

We use the st2client package fairly extensively to facilitate tight integration between some of our internal python services and st2. This PR is one of multiple which extend st2client to wrap more of the existing st2api endpoints which are already implemented.

Random thought - is there a way we can "sync" the api and the client? ie ensure that all of the api endpoints have a corresponding client method implemented?

@pull-request-size pull-request-size bot added the size/S PR that changes 10-29 lines. Very easy to review. label Sep 17, 2019
@nmaludy
Copy link
Member

nmaludy commented Sep 24, 2019

On the "sync" stuff, i know i've brought it up before. I thought about generating the client from the swagger spec.

However, there is some "custom" code that gets added on the client side for some of the stuff to make the human output "pretty".

Would still be nice if there was a way for the end-client CLI to get all of the new features/flags and endpoints, even if it's basic support at first.

@arm4b arm4b requested a review from m4dcoder September 24, 2019 17:48
Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

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

Changelog is missing. Would be good to have one for this fix.

Copy link
Contributor

@m4dcoder m4dcoder left a comment

Choose a reason for hiding this comment

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

The change looks good in general. But please add unit tests. Please also update the details of this PR to provide summary and intent of the change. Why is this change needed? What is the expected outcome after the change? Thanks.

@pull-request-size pull-request-size bot added size/L PR that changes 100-499 lines. Requires some effort to review. and removed size/S PR that changes 10-29 lines. Very easy to review. labels Sep 24, 2019
Copy link
Contributor

@m4dcoder m4dcoder left a comment

Choose a reason for hiding this comment

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

Minor addition required for unit tests.


endpoint = '/actions/views/entry_point/%s' % EXECUTION['id']

httpclient.HTTPClient.get.assert_called_with(endpoint)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a unit tests where the status code from the API call is not OK/200.

Copy link
Member Author

Choose a reason for hiding this comment

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

Pushed a commit for this, but not sure if I implemented it correctly, please let me know if I should take a different approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use assertRaisesRegexp and check that the error message is actually Not Found returned in the FakeResponse? Search for assertRaisesRegexp in st2 and you will see plenty of examples.

Copy link
Member Author

Choose a reason for hiding this comment

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

@m4dcoder sorry for the delay, I've pushed a commit to use that method. Please let me know if it's not correct.


endpoint = '/actions/views/entry_point/%s' % EXECUTION['id']

httpclient.HTTPClient.get.assert_called_with(endpoint)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use assertRaisesRegexp and check that the error message is actually Not Found returned in the FakeResponse? Search for assertRaisesRegexp in st2 and you will see plenty of examples.

Copy link
Contributor

@m4dcoder m4dcoder left a comment

Choose a reason for hiding this comment

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

LGTM. Need CI jobs to pass before this can be merged.

@m4dcoder
Copy link
Contributor

m4dcoder commented Oct 8, 2019

@trstruth You also need to rebase this branch. There is a merge conflict.

@arm4b arm4b added this to the 3.2.0 milestone Oct 8, 2019
@trstruth
Copy link
Member Author

trstruth commented Oct 8, 2019

@m4dcoder I've resolved the conflict, let's see if the ci builds pass this time

@m4dcoder m4dcoder merged commit a643ba7 into StackStorm:master Oct 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L PR that changes 100-499 lines. Requires some effort to review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants