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

[Bug][API]edit workflow query resources list error. #12664

Closed
wants to merge 1 commit into from

Conversation

hstdream
Copy link
Contributor

@hstdream hstdream commented Nov 2, 2022

Purpose of the pull request

close #12573

Brief change log

image

Verify this pull request

edit workflow

@codecov-commenter
Copy link

codecov-commenter commented Nov 2, 2022

Codecov Report

Merging #12664 (a6af154) into dev (9e0c9af) will increase coverage by 0.01%.
The diff coverage is 16.66%.

@@             Coverage Diff              @@
##                dev   #12664      +/-   ##
============================================
+ Coverage     39.04%   39.06%   +0.01%     
- Complexity     4185     4186       +1     
============================================
  Files          1043     1043              
  Lines         39511    39515       +4     
  Branches       4540     4541       +1     
============================================
+ Hits          15428    15436       +8     
+ Misses        22324    22318       -6     
- Partials       1759     1761       +2     
Impacted Files Coverage Δ
...heduler/api/service/impl/ResourcesServiceImpl.java 37.45% <16.66%> (-0.25%) ⬇️
...r/plugin/registry/zookeeper/ZookeeperRegistry.java 50.80% <0.00%> (+7.25%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@sonarcloud
Copy link

sonarcloud bot commented Nov 2, 2022

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

37.5% 37.5% Coverage
0.0% 0.0% Duplication

@caishunfeng
Copy link
Contributor

I found that this pr can not resolve the issue, when I want to add new shell task, it will query resources and throw error too.

I think we should change the front part, if fullName is empty, don't request resource list.
image

@caishunfeng caishunfeng added the bug Something isn't working label Nov 2, 2022
@caishunfeng caishunfeng added this to the 3.2.0 milestone Nov 2, 2022
@hstdream
Copy link
Contributor Author

hstdream commented Nov 3, 2022

I found that this pr can not resolve the issue, when I want to add new shell task, it will query resources and throw error too.

I think we should change the front part, if fullName is empty, don't request resource list. image

Yours is another request, and reporting an error is not a problem with me

@caishunfeng
Copy link
Contributor

I found that this pr can not resolve the issue, when I want to add new shell task, it will query resources and throw error too.
I think we should change the front part, if fullName is empty, don't request resource list. image

Yours is another request, and reporting an error is not a problem with me

related pr #12076 cc @SbloodyS @EricPyZhou

@EricPyZhou
Copy link
Contributor

@hstdream please confirm two things:

  1. Do you login as admin user?
  2. Do all tenantCodes used by all users have its path defined in your environment? For example, if you are using S3 bucket, verify that if tenantCode/resources/ exist
    If not, then it will throw this error.

@calvinjiang
Copy link
Contributor

@hstdream please confirm two things:

  1. Do you login as admin user?
  2. Do all tenantCodes used by all users have its path defined in your environment? For example, if you are using S3 bucket, verify that if tenantCode/resources/ exist
    If not, then it will throw this error.

@hstdream @caishunfeng I think one of the most important issues is the error message. That couldn't tell users what the exact problem is. The error message should tell users like 'You need to set the resource storage in the file of common.properties.'

@EricPyZhou
Copy link
Contributor

@hstdream please confirm two things:

  1. Do you login as admin user?
  2. Do all tenantCodes used by all users have its path defined in your environment? For example, if you are using S3 bucket, verify that if tenantCode/resources/ exist
    If not, then it will throw this error.

@hstdream @caishunfeng I think one of the most important issues is the error message. That couldn't tell users what the exact problem is. The error message should tell users like 'You need to set the resource storage in the file of common.properties.'

@hstdream please confirm two things:

  1. Do you login as admin user?
  2. Do all tenantCodes used by all users have its path defined in your environment? For example, if you are using S3 bucket, verify that if tenantCode/resources/ exist
    If not, then it will throw this error.

@hstdream @caishunfeng I think one of the most important issues is the error message. That couldn't tell users what the exact problem is. The error message should tell users like 'You need to set the resource storage in the file of common.properties.'

#12901 now in this issue, we propose to return empty list when a tenant resource path is not found.

@davidzollo
Copy link
Contributor

@hstdream please confirm two things:

  1. Do you login as admin user?
  2. Do all tenantCodes used by all users have its path defined in your environment? For example, if you are using S3 bucket, verify that if tenantCode/resources/ exist
    If not, then it will throw this error.

@hstdream @caishunfeng I think one of the most important issues is the error message. That couldn't tell users what the exact problem is. The error message should tell users like 'You need to set the resource storage in the file of common.properties.'

+1, The error message should tell users like 'You need to set the resource storage in the file of common.properties.' is a better suggestion @hstdream what do you think?

Comment on lines +851 to +852
logger.error("Storage does not start up, resource upload startup state: {}.",
PropertyUtils.getResUploadStartupState());
Copy link
Contributor

Choose a reason for hiding this comment

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

The error message should tell users like 'You need to set the resource storage in the file of common.properties.'

@EricPyZhou
Copy link
Contributor

EricPyZhou commented Nov 15, 2022

@hstdream please confirm two things:

  1. Do you login as admin user?
  2. Do all tenantCodes used by all users have its path defined in your environment? For example, if you are using S3 bucket, verify that if tenantCode/resources/ exist
    If not, then it will throw this error.

@hstdream @caishunfeng I think one of the most important issues is the error message. That couldn't tell users what the exact problem is. The error message should tell users like 'You need to set the resource storage in the file of common.properties.'

+1, The error message should tell users like 'You need to set the resource storage in the file of common.properties.' is a better suggestion @hstdream what do you think?

Please shift discussion into #12901 as it is the dedicated one for designing implementations on resolving this issue.

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity for 120 days. It will be closed in 7 days if no further activity occurs.

@github-actions github-actions bot added the Stale label Mar 16, 2023
@github-actions
Copy link

github-actions bot commented Apr 5, 2023

This pull request has been closed because it has not had recent activity. You could reopen it if you try to continue your work, and anyone who are interested in it are encouraged to continue work on this pull request.

@github-actions github-actions bot closed this Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend bug Something isn't working Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug][API]edit workflow query resources list error.
6 participants