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.

Sign up for GitHub

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

Jump to bottom

Added call to version endpoint of api, added basic logging #20 #36

Merged
merged 25 commits into from Mar 30, 2022
Merged

Added call to version endpoint of api, added basic logging #20 #36

merged 25 commits into from Mar 30, 2022

Conversation

S4nvers
Copy link
Contributor

@S4nvers S4nvers commented Mar 20, 2022 •
edited by MrF3lix

In addition to implementing the basic api call, I also added some logging functionality. But if you guys think this is not necessary I'll remove it again.

Related Issues

@S4nvers
Copy link
Contributor Author

S4nvers commented Mar 20, 2022

Oh and if someone has any tips on how to write tests for the index.ts file for example I'd be glad to hear them

@MrF3lix MrF3lix added enhancement New feature or request executor This issue is related to the executor labels Mar 20, 2022
MrF3lix
MrF3lix reviewed Mar 20, 2022
View reviewed changes
Copy link
Contributor

@MrF3lix MrF3lix left a comment

Choose a reason for hiding this comment

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

Sorry to leave my unrequested comments, I just have a lot of time to look over it.

.vscode/launch.json Outdated Show resolved Hide resolved
src/executor/source/ApiInterface.ts Outdated Show resolved Hide resolved
src/executor/source/ApiInterface.ts Outdated Show resolved Hide resolved
src/executor/source/index.ts Outdated Show resolved Hide resolved
src/executor/source/routes/VersionRoute.ts Outdated Show resolved Hide resolved
src/executor/source/utils/logging/Logger.ts Outdated Show resolved Hide resolved
src/executor/source/utils/logging/morganMiddleware.ts Outdated Show resolved Hide resolved
@MrF3lix
Copy link
Contributor

MrF3lix commented Mar 20, 2022 •
edited

@S4nvers

Oh and if someone has any tips on how to write tests for the index.ts file for example I'd be glad to hear them

I think it's ok to exclude the index.ts file from the code coverage. The only test that would be reasonable here would be to test if the app listens on port 3500 and that is subject to change anyway.

@sonarcloud
Copy link

sonarcloud bot commented Mar 24, 2022

[API] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@S4nvers S4nvers marked this pull request as ready for review March 24, 2022 08:12
@S4nvers S4nvers requested a review from MrF3lix March 24, 2022 08:12
@sonarcloud
Copy link

sonarcloud bot commented Mar 25, 2022

[Editor] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sonarcloud
Copy link

sonarcloud bot commented Mar 25, 2022

[Executor] Please retry analysis of this Pull-Request directly on SonarCloud.

@sonarcloud
Copy link

sonarcloud bot commented Mar 25, 2022

[Executor] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

riesentoaster
riesentoaster requested changes Mar 25, 2022
View reviewed changes
Copy link
Contributor

@riesentoaster riesentoaster left a comment •
edited

Choose a reason for hiding this comment

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

basic logging – well two thirds of the code is logging now. And I don't know the library, so no comment there.

I've got a few questions, take a look.

Also: How does logging work in the k8s cluster? We'd need some persistence etc. Create an issue for this maybe?

Also: Do we document methods/functions somehow (like a JavaDoc equivalent)?

src/executor/source/ApiInterface.ts Outdated Show resolved Hide resolved
src/executor/source/Index.ts Show resolved Hide resolved
src/executor/source/Index.ts Show resolved Hide resolved
src/executor/source/routes/VersionRouter.ts Outdated Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud bot commented Mar 30, 2022

[Executor] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@S4nvers S4nvers merged commit 9d5952d into main Mar 30, 2022
@S4nvers S4nvers deleted the feature/ExecutorAPICall branch March 30, 2022 08:52
@riesentoaster riesentoaster mentioned this pull request Mar 31, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MrF3lix MrF3lix MrF3lix approved these changes

@riesentoaster riesentoaster riesentoaster approved these changes

@Geissemanu Geissemanu Awaiting requested review from Geissemanu

Assignees

@S4nvers S4nvers

Labels
enhancement New feature or request executor This issue is related to the executor
Projects
Development V1
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Executor should call API
3 participants
@S4nvers @MrF3lix @riesentoaster