-
Notifications
You must be signed in to change notification settings - Fork 272
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
Add postman collection and README for general workflow #782
Conversation
I see a heartbeat_demo test is failing, would you like to fix that and I can merge it into this PR? |
Hey @mallek and team - thanks for this! We're working on merging in some other pull requests but will try to give this our attention in the next several days. |
"_postman_variable_scope": "environment", | ||
"_postman_exported_at": "2019-02-18T16:37:46.501Z", | ||
"_postman_exported_using": "Postman/6.5.2" | ||
} |
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.
missing newline at EOF
Awesome! leaving this open for a bit as we investigate how to include the changeset in CI. |
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.
Everything looks good here, but my only concern is that endpoints get updated and the postman examples get left behind.
Currently, no one on the team actively uses postman. With that said, that's my only concern with this PR. If a member of the team decided to maintain this, then I think I could be +1 to this contribution. Due to the early stages of this API as well, I'm hesitant to agree to merge this in such a state. If the API was more frozen, then I can see merging this even if no one on the team was going to use postman.
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.
Perhaps we can integrate in the CI workflow some command-line tool for postman, such as Newman.
I had also suggested a tool like newman to run the tests. I like making the calls because it allows the http context to be generated without any mocking. Because of that his will also be a good place to test things like CORS access or TLS if that is intended to be added to the product. I also think having a postman collection may be easy for a first time user to pick up without having to read and understand the python tests. I do want to make sure the documentation stays up to date. I think this would be done if it is truly used by the community where if it's not used it will be neglected. |
I actually do use postman, and I think this is a nice step for lowering the bar to entry; However, our experience tells us that anything that is untested will eventually fail. I'm looking forward to having this contribution merged if we can find a way to include in in CI. |
We have identified that newman would be an appropriate way to run these tests. What additional work would you like to see done on this PR? |
@mallek can you prepare a python script that runs it? We can use that as a basic for tests integrated in our CI |
@cygnusv. Newman is a command line tool. You want me to use python to start the cli tool? The way I have done this is run it as a step in CI shelling out to run Newman. |
No need to use python to run the CLI, I think. Just specify what are the newman commands we need to run, and we can integrate them in the CI workflow. |
Is there a set of running actors available to hit via http during the CI process? |
We prefer to deploy them locally in the test workflow. See this test as an example, which deploys a fleet or Ursulas to test the heartbeat demo Line 444 in ec6b754
|
"response": [] | ||
} | ||
] | ||
} |
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.
Missing Newline at OEF
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.
The HTTP Character Control API has been updated. For example "signing_key" is now "verifying_key" in all cases.
I rebased this PR from #820 and made the changes requested |
Heh - looks like you picked up an extra 172 commits there! (perhaps due to diverged history.) |
Interesting I show a clean rebase from master. I haven't worked much with upstream only origin so i'll play around with it and see if i can get it. If not since it's just documentation I can just create a new PR from master |
Be sure to add |
that was it, I rebased over origin/master I'll try it... thanks |
85b8189
to
82a3815
Compare
This should be much better, sorry :| |
No worries! :-) |
something in my tooling keeps removing the new line.. |
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.
This will need automated testing - so we'll follow up with an Issue. Approved - Thanks for the contribution!
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.
🐧
Issue #840 |
Co-Authored-By: mallek <travis@haleycomputersolutions.com>
I added the postman collection we used at ETHDenver to the /examples/postman and added a quick readme to describe the workflow