-
Notifications
You must be signed in to change notification settings - Fork 2
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
ci: add workflows #34
Conversation
Reviewer's Guide by SourceryThis pull request introduces a new module for Task Execution Service (TES) with task management and async functions, a Transport module for HTTP requests, and a ServiceInfo module for service information retrieval. It also includes GitHub Actions workflows for PR evaluation, code testing, and code quality checks, along with configuration files for markdownlint, pre-commit hooks, and yamllint. Additionally, scripts and configuration for building OpenAPI models and running tests are added. File-Level Changes
Tips
|
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.
Some minor comments, but generally the workflows look really good/useful. Of course needs some changes in the code to pass. Also, perhaps a more Rust-experienced person could look at it, because I don't know the toolchain really. @pavelnikonorov @vemonet
I have added CODECOV_TOKEN
.
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.
Doesn't have anything to do with 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.
😭 yeah I just wanted to add most of the things that were left from cookiecutter thats it, not just the 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.
Fair. But let's make sure to remove this from this PR and add it to another (together with the PR template).
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.
committed the removal of .github/ISSUE_TEMPLATE/general-purpose.md
to the branch
with: | ||
component: clippy | ||
|
||
- name: Run unit tests |
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.
Run linting?
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.
captions updated to the format "Running .... checks"
run: cargo test --tests | ||
|
||
- name: Install cargo-tarpaulin | ||
run: cargo install cargo-tarpaulin |
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.
Add to Cargo.toml
?
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.
aah yes, didn't touch that as it wan't merged.
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.
I think since it is a CLI tool it needs to be installed manually outside of the Cargo.toml
, or maybe you can add it to the rust-toolchain file
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.
Really? What's the difference? Aren't the deps in Cargo.toml
installed via cargo install
?
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.
@uniqueg deps in Cargo.toml
are installed differently – into the project's build cache, while the cargo install
is only for standalone CLI tools such as rustfmt
and cargo-tarpaulin
.
as for cargo-tarpaulin
, it can be installed via components
like clippy
, but only with the 'nightly' toolchain. So I’d suggest keeping it as a separate installation step.
Adding a rust-toolchain
might be beneficial, so I've added an issue #45 (comment)
poetry-install-options: "--only=misc --no-root" | ||
poetry-export-options: "--only=misc" | ||
|
||
- name: Check all the pre-commit hooks passed |
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.
Check that all pre-commit hooks passed
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 is what I have in cookiecutter though!!!! Do I have change it there as well?
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 the need for poetry here?
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.
Please ignore, was copying the structure from a python project, must have been left in. Will remove.
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.
Yes, not a biggie of course. You can change it here, and sneak it in some other time in the cookiecutter template, if you remember
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.
Ah, yes, overlooked the Poetry artifacts here - good catch @vemonet
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.
committed the removal of Poetry
@uniqueg either merge this after we have merged all other PRs, because resolving convo will mean me changing some code so it doesn't make sense to do that rn, or merge this into some PR take it from there which might not be ideal though. |
.github/workflows/code_test.yaml
Outdated
run: cargo install cargo-tarpaulin | ||
|
||
- name: Run unit tests with coverage | ||
run: cargo tarpaulin --tests --out Xml |
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.
I think you should also add the --doc
flag so that tarpaulin includes codeblocks from the docstrings
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.
committed the adding of the --doc
flag in both integration in unit tests
Looking good, most things required seems to be there: tests, cov, fmt, linting, precommit |
Thanks for the review, Ill make changes when the branch on which this is bases off of gets merged. |
"This branch has no conflicts with the base branch" <- I think you could already make the changes? Of course, it may need some other stuff until tests pass, but I do think you could work towards resolving the open issues at this point, @JaeAeich and @aaravm |
792e8da
to
7442c57
Compare
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.
@uniqueg I've reviewed all conversations, and committed the discussed changes.
also, I have tested it locally using act
, the workflows work, but the tests fail, which should be addressed separately and within the other PRs review.
Added two issues out of this PR: #44, #45
P.S. FYI, I've done a few push-force on my commits here to amend them, but it appears that it uses a different credentials source, which belongs to Grigorii who used to work on the same machine.
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.
committed the removal of .github/ISSUE_TEMPLATE/general-purpose.md
to the branch
with: | ||
component: clippy | ||
|
||
- name: Run unit tests |
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.
captions updated to the format "Running .... checks"
run: cargo test --tests | ||
|
||
- name: Install cargo-tarpaulin | ||
run: cargo install cargo-tarpaulin |
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.
@uniqueg deps in Cargo.toml
are installed differently – into the project's build cache, while the cargo install
is only for standalone CLI tools such as rustfmt
and cargo-tarpaulin
.
as for cargo-tarpaulin
, it can be installed via components
like clippy
, but only with the 'nightly' toolchain. So I’d suggest keeping it as a separate installation step.
Adding a rust-toolchain
might be beneficial, so I've added an issue #45 (comment)
poetry-install-options: "--only=misc --no-root" | ||
poetry-export-options: "--only=misc" | ||
|
||
- name: Check all the pre-commit hooks passed |
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.
committed the removal of Poetry
.github/workflows/code_test.yaml
Outdated
run: cargo install cargo-tarpaulin | ||
|
||
- name: Run unit tests with coverage | ||
run: cargo tarpaulin --tests --out Xml |
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.
committed the adding of the --doc
flag in both integration in unit tests
all suggestions addressed, merging as discussed with @uniqueg |
Summary by Sourcery
Introduce a new Task Execution Service (TES) module along with supporting modules for transport, configuration, and service information. Add comprehensive CI workflows for PR evaluation, code testing, and code quality checks. Implement build and test scripts, and add documentation and issue templates.
New Features:
Enhancements:
Build:
CI:
Documentation:
Tests:
Chores: