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

Issue #517 - TCP Input Support #521

Closed
wants to merge 0 commits into from
Closed

Conversation

cjjacks
Copy link
Contributor

@cjjacks cjjacks commented Mar 14, 2024

  • Adds a TCP client and server option to input streams
  • Updates docunmentation with new input specifications
  • Adds client specific tests
  • Adds additional stream tests
  • Formatting

@cjjacks cjjacks requested review from a team as code owners March 14, 2024 16:47
@cjjacks
Copy link
Contributor Author

cjjacks commented Mar 14, 2024

Hopefully I've got this PR set up right - I'm more used to Gitlab which has slightly different mechanics. Also, I apologize for the large number of changes - a large portion of the changes are just formatting updates from merging #519.

@cjjacks
Copy link
Contributor Author

cjjacks commented Jul 26, 2024

Sorry for the delay in getting this ready to review. I got side tracked with some other projects. Here is a summary of what I have done in this pr:

- Adds a TCP client and server option to input streams
- Adds option to use 0.0.0.0 for server binding
- Adds TCP to output clients and the ability to send to a remote host
- Adds factory methods to determine the correct stream to instantiate based on config
- Updates documentation with new input/output specifications
- Adds client specific tests
- Adds additional stream tests
- Repo wide formatting from bringing in pre-commit changes
- Temporarily disables python 3.7 tests
- add a docker compose stack to test network configs

Also, sorry for the large amount of changes in this PR. The vast majority of changes are just auto formatting done by running:

pre-commit run -a

This brings the enitre repo in line with the linter and not just the diff. If its too overwhelming i can back these changes out of the PR.

As far as verification, I have added some tests as well as docker compose stack to test the different clients/servers. If you have make, docker and docker compose. you can run the following:

make network-test

and this will do some stress testing with all the new input/output stream features. Additionally, I have been using this branch and specifically the TCPInputStream for some time now as part of developing with AIT Core

@cjjacks cjjacks marked this pull request as ready for review July 26, 2024 21:51
@nttoole
Copy link
Contributor

nttoole commented Sep 17, 2024

@cjjacks Sorry for our extnsive delay, our time has been limited and what little there is has been spent investigating separate security issues.

So some notes thus far (with requested changes):

  1. Linter updates: Looks like we would prefer for them to be their own PR, preferably before the TCP changes. So could you please separate those?

  2. New constants: ait.core.constants should be in ait.core.init with all the other constants. Or, at the very least, all the constants should get moved into constants.py and then exposed in the init.py file for consistency

  3. “Address specification”: the syntax breaks the existing convention so could this be revisited. At the very least,

            - "UDP"
            - "server"
            - 3077

would be better as 'UDP:server:3077’, or something similar.

  1. The Docker/makefile may need to be their own PRs, so that the TCP update is better isolated

  2. OutputClient: Currently it is supporting both UDP and TCP cases. Would there be a way to cleanly separate these into separate classes, and update 'PortOutputStream' extensions accordingly?

Thanks,
Nicko

Copy link

sonarcloud bot commented Sep 18, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
2 Security Hotspots
E Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

USER ait
WORKDIR $PROJECT_HOME
COPY --chown=${USER}:${GROUP} . $PROJECT_HOME/AIT-Core
RUN python3.9 -m pip install --user --upgrade pip setuptools virtualenvwrapper virtualenv poetry \

Check notice

Code scanning / SonarCloud

Arguments in long RUN instructions should be sorted

<!--SONAR_ISSUE_KEY:AZIHLfb6dROWO1Ivlh9J-->Sort these package names alphanumerically. <p>See more on <a href="https://sonarcloud.io/project/issues?id=NASA-AMMOS_AIT-Core&issues=AZIHLfb6dROWO1Ivlh9J&open=AZIHLfb6dROWO1Ivlh9J&pullRequest=521">SonarCloud</a></p>
@cjjacks
Copy link
Contributor Author

cjjacks commented Sep 26, 2024

Sorry for the delay again. I'll get to work addressing the feedback above!

Copy link

sonarcloud bot commented Oct 23, 2024

@cjjacks cjjacks deleted the issue-517 branch October 23, 2024 20:07
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.

2 participants