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

Http client #522

Merged
merged 41 commits into from
Jun 28, 2024
Merged

Http client #522

merged 41 commits into from
Jun 28, 2024

Conversation

yassiezar
Copy link
Contributor

@yassiezar yassiezar commented Aug 2, 2023

┆Issue is synchronized with this Jira Task by Unito

@yassiezar
Copy link
Contributor Author

No issues, but I'll probably need to modify it to make it a bit more generic so that its useful to other users

@brettpac
Copy link
Collaborator

brettpac commented Aug 4, 2023

Are there any complications with...

Moving the client https://github.com/yassiezar/SMACC2/blob/http-client/smacc2/include/smacc2/client_bases/smacc_http_client.hpp, to it's own folder in the smacc_client_library folder?
Moving the code to a cpp file?

Let's chat offline about this.

@yassiezar
Copy link
Contributor Author

@brettpac I moved the packages and made separate CPP files. I still want to modify the client to be more generically useful

Copy link
Collaborator

Choose a reason for hiding this comment

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

The only thing left to do is move this file into a client behaviors folder in the http client directory.

@yassiezar
Copy link
Contributor Author

@brettpac just to update you, the example isn't working as expected anymore and HTTP responses aren't being received. I'm busy investigating, but I suspect it has to do with changes to Boost::beast's APIs. I'll ping you when its resolved

yassiezar and others added 12 commits June 4, 2024 21:16
…compilation units. Modified examples to use this new client
Pretty sure this change was made accidently.
Added License to file to clean up format error
Found better Copyright & License for CI Format error.
Another little format error
Somehow a bracket became a cNode
- Fixed response callback not triggering correctly and causing a crash
- Fixed example not transitioning back to state 1 after a response has been received
yassiezar and others added 10 commits June 4, 2024 21:30
…compilation units. Modified examples to use this new client
Added License to file to clean up format error
Found better Copyright & License for CI Format error.
Another little format error
Somehow a bracket became a cNode
- Fixed response callback not triggering correctly and causing a crash
- Fixed example not transitioning back to state 1 after a response has been received
@brettpac
Copy link
Collaborator

brettpac commented Jun 5, 2024

I think it's just being held up by precommit...

Command: pre-commit run -a

@brettpac
Copy link
Collaborator

brettpac commented Jun 6, 2024

Hi @yassiezar , thank you for this pull request. Looks awesome.
One last request...

For the cb_http_request found here...
smacc2_sm_reference_library/sm_atomic_http/include/sm_atomic_http/clients/client_behaviors

Let's move that into a new folder entitled "client_behaviors" and place that folder inside of this one...
smacc2_client_library/http_client/include/http_client/

So the end result would look like...
smacc2_client_library/http_client/include/http_client/client_behaviors/cb_http_request.hpp

Thank you,

@brettpac
Copy link
Collaborator

Hi Jaycee, looks like we're missing some copyrights...
smacc2_client_library/http_client/src/http_client/http_session.cpp: could not find copyright notices...

1 errors, checked 306 files
smacc2_client_library/http_client/src/http_client/ssl_http_session.cpp: could not find copyright notice
1 errors, checked 296 files
smacc2_client_library/http_client/include/http_client/ssl_http_session.hpp: could not find copyright notice
1 errors, checked 274 files
smacc2_client_library/http_client/include/http_client/http_session.hpp: could not find copyright notice
smacc2_client_library/http_client/include/http_client/http_session_base.hpp: could not find copyright notice

@brettpac
Copy link
Collaborator

// Copyright 2021 RobosoftAI Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

/*****************************************************************************************************************
*

Author: Jacobus Lock

******************************************************************************************************************/

@brettpac
Copy link
Collaborator

Hmmm. As Im reading it, the failed binary check is tied to the sm_simple_action_client state machine package I just merged a few minutes ago...

@yassiezar
Copy link
Contributor Author

@brettpac I just merged in your changes. That might fix the error

@@ -63,7 +63,7 @@ repos:
- id: clang-format
name: clang-format
description: Format files with ClangFormat.
entry: clang-format-12
entry: clang-format
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@brettpac I replaced the usage of the specific clang-format version with the generic executable. Currently Ubuntu installs clang-format 14

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nevermind, I see the CI container is specifically looking for clang-format-12

@brettpac
Copy link
Collaborator

I think to get this passing we just have to run precommit twice. Once to make fixes (which I can see it does from the logs), and the other to approve.

@yassiezar
Copy link
Contributor Author

All the checks pass when i run it locally, not sure how to get it to pass here. I'm not sure why the clang-format check is failing above, does the command need to be edited to use clang-format instead of clang-format-12? Ubuntu 22.04 uses clang-format-14

@brettpac brettpac merged commit 682e73c into robosoft-ai:humble Jun 28, 2024
8 checks passed
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.

None yet

2 participants