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

fix: Resource relate path invalid when tenant change #15581

Merged
merged 1 commit into from
Feb 6, 2024

Conversation

zhongjiajie
Copy link
Member

  • fix can not get correct resource related path when user run workflow with differnet tenant of resource created
  • also fix can not get correct related path when we use resource.storage.type=LOCAL

Purpose of the pull request

Brief change log

Verify this pull request

This pull request is code cleanup without any test coverage.

(or)

This pull request is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(or)

If your pull request contain incompatible change, you should also add it to docs/docs/en/guide/upgrede/incompatible.md

* fix can not get correct resource related path when
  user run workflow with differnet tenant of resource
  created
* also fix can not get correct related path when we
  use `resource.storage.type=LOCAL`
Copy link
Member Author

@zhongjiajie zhongjiajie left a comment

Choose a reason for hiding this comment

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

maybe we should total remove tenant attribute when we CURD resource later

Comment on lines +35 to +36
// prefix schema `file:/` should be remove in local file mode
String fullNameRemoveSchema = fullName.replaceFirst(hdfsProperties.getDefaultFS(), "");
Copy link
Member Author

Choose a reason for hiding this comment

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

fix get related path not work in local resources mode

Comment on lines +82 to +86
// Replace resource dir not effective in case of run workflow with different tenant from resource file's.
// this is backup solution to get related path, by split with RESOURCE_TYPE_FILE
return filenameReplaceResDir.contains(RESOURCE_TYPE_FILE)
? filenameReplaceResDir.split(String.format("%s/", RESOURCE_TYPE_FILE))[1]
: filenameReplaceResDir;
Copy link
Member Author

Choose a reason for hiding this comment

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

fix error when execute workflow with different tenant of resource's creator

@zhongjiajie zhongjiajie self-assigned this Feb 6, 2024
@zhongjiajie zhongjiajie added this to the 3.2.1 milestone Feb 6, 2024
Copy link

sonarcloud bot commented Feb 6, 2024

Quality Gate Failed Quality Gate failed

Failed conditions

6.7% Coverage on New Code (required ≥ 60%)

See analysis details on SonarCloud

@EricGao888
Copy link
Member

maybe we should total remove tenant attribute when we CURD resource later

+1, resource dir connected with tenant code is kind of outdated.

@zhongjiajie
Copy link
Member Author

maybe we should total remove tenant attribute when we CURD resource later

+1, resource dir connected with tenant code is kind of outdated.

thanks, but you sleep so later, nightbird 🌔 🐱

Copy link
Member

@EricGao888 EricGao888 left a comment

Choose a reason for hiding this comment

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

Better if we have it covered with unit test.

@codecov-commenter
Copy link

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (89da67d) 38.53% compared to head (a2d7478) 38.52%.

❗ Current head a2d7478 differs from pull request most recent head 97139f1. Consider uploading reports for the commit 97139f1 to get more accurate results

Files Patch % Lines
...uler/plugin/storage/hdfs/LocalStorageOperator.java 0.00% 7 Missing ⚠️
...ugin/storage/hdfs/LocalStorageOperatorFactory.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##                dev   #15581      +/-   ##
============================================
- Coverage     38.53%   38.52%   -0.01%     
- Complexity     4765     4766       +1     
============================================
  Files          1305     1306       +1     
  Lines         44845    44852       +7     
  Branches       4808     4808              
============================================
+ Hits          17279    17280       +1     
- Misses        25687    25694       +7     
+ Partials       1879     1878       -1     

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

@zhongjiajie
Copy link
Member Author

zhongjiajie commented Feb 6, 2024

Better if we have it covered with unit test.

Could we add later? I wish I could raise release vote tonight and I can stay with my family in Chinese new year

@zhongjiajie zhongjiajie added bug Something isn't working ready-to-merge labels Feb 6, 2024
@zhongjiajie zhongjiajie merged commit bd83631 into apache:dev Feb 6, 2024
55 of 56 checks passed
@zhongjiajie zhongjiajie deleted the resource_local branch February 6, 2024 15:56
zhongjiajie added a commit that referenced this pull request Feb 6, 2024
* fix can not get correct resource related path when
  user run workflow with differnet tenant of resource
  created
* also fix can not get correct related path when we
  use `resource.storage.type=LOCAL`

(cherry picked from commit bd83631)
@EricGao888
Copy link
Member

Better if we have it covered with unit test.

Could we add later? I wish I could raise release vote tonight and I can stay with my family in Chinese new year

Sure, happy new year : )

@zhongjiajie
Copy link
Member Author

thanks, happy new year

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ready-to-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants