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

[YUNIKORN-2135] Add integration test code coverage #733

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

FrankYang0529
Copy link
Member

@FrankYang0529 FrankYang0529 commented Nov 22, 2023

What is this PR for?

If ENABLE_GO_COVER_DIR is TRUE:

  • Add cover flags to go build.
  • Use root user to build docker images.
  • Add uninstall_yunikorn and copy_go_cover_dir to run-e2e-tests.sh.

What type of PR is it?

  • - Improvement

Todos

What is the Jira issue?

https://issues.apache.org/jira/browse/YUNIKORN-2135

@FrankYang0529 FrankYang0529 self-assigned this Nov 22, 2023
@FrankYang0529 FrankYang0529 force-pushed the YUNIKORN-2135 branch 2 times, most recently from c3a62c7 to 470b53b Compare November 26, 2023 05:16
Copy link

codecov bot commented Nov 26, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c5a82ac) 69.43% compared to head (3f1e58d) 77.30%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #733      +/-   ##
==========================================
+ Coverage   69.43%   77.30%   +7.86%     
==========================================
  Files          50       73      +23     
  Lines        7993     9284    +1291     
==========================================
+ Hits         5550     7177    +1627     
+ Misses       2254     1848     -406     
- Partials      189      259      +70     
Flag Coverage Δ
e2e 63.79% <ø> (?)
unittests 69.43% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pbacsko
Copy link
Contributor

pbacsko commented Feb 9, 2024

@FrankYang0529 could you resolve the merge conflicts?

@craigcondit
Copy link
Contributor

@FrankYang0529 can we write the logs to a directory under /tmp in the container? I'm not a fan of a top-level dir for this...

@@ -87,7 +87,8 @@ function install_cluster() {
if [ "${GIT_CLONE}" = "true" ]; then
check_cmd "git"
rm -rf ./build/yunikorn-release
git clone --depth 1 https://github.com/apache/yunikorn-release.git ./build/yunikorn-release
# TODO: update following branch if PR in yunikorn-release is merged
git clone --depth 1 https://github.com/FrankYang0529/yunikorn-release.git -b YUNIKORN-2135 ./build/yunikorn-release
Copy link
Contributor

Choose a reason for hiding this comment

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

As indicated in the yunikorn-release repo PR, I think we should find a way to deploy this without polluting the upstream helm charts.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here's the approach we should use: https://helm.sh/docs/topics/advanced/#post-rendering

Helm supports a post-renderer that can be used to update the generated output. We can use this in the e2e test script during install to "patch" the installed charts with what is required here. An ideal post-renderer would be kustomize (see https://kustomize.io/). An example using the two together can be found here: https://github.com/thomastaylor312/advanced-helm-demos/tree/master/post-render

We can setup a kustomize script to patch only the objects we need so that the original helm charts can stay pristine. All that should be required is an ENV var and build-mount for /tmp/coverage (or something like it).

Copy link
Contributor

@pbacsko pbacsko left a comment

Choose a reason for hiding this comment

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

@FrankYang0529 could you rebase the changes please? I'll come back to this once it can be merged.

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.

3 participants