-
Notifications
You must be signed in to change notification settings - Fork 1
added shell2http for CBS and SGW #338
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
base: main
Are you sure you want to change the base?
Conversation
borrrden
left a comment
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.
Why is this not an addition to the existing edge server shell2http branch I wonder?
|
|
||
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
|
|
||
| setsid /home/ec2-user/shell2http/shell2http -no-index -cgi -500 -port 20003 \ |
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.
It's going to be confusing having these all on different ports. Don't think of them as specific to an install, just think of them as an overall service that should have a defined port.
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.
Oh? Wait I somehow thought that each process shall run on different port 🤔 , now that you say it, it doesn't have to, the same process can just call different scripts be it ES, SG or CBS.
That's what you mean right?
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.
Also yes I can move changes to that branch, why its not currently, is because when starting I wanted it to be reviewed and merged at my own time convenience since there is another branch having the tests relying on these changes. If that branch is ready to merge I can move all this there?
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.
What is this going to be used for?
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.
To get the status of CBS node while its been called to be on/off, since that'd by wrapped in _try_n_times instead of a while loop or waiting an arbitrary amount of time.
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.
Parsing out unformatted text is not a great way to do this. If you are going to serve this to a consumer, make it JSON or some other serialized format.
Added this PR with Shell2Http changes so that the #335 can be merged, which needs a SGW restart for the second LOAD BALANCER test to fulfill its purpose