-
Notifications
You must be signed in to change notification settings - Fork 344
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
[CDAP-21089] OAuthHandler should not return 5xx on 4xx errors to skip retries and throw errors for invalid macros based on stage validation request. #15758
Conversation
5623990
to
04bf38c
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.
Add unit test for the newly added option in MacroParser
.
Additionally add screenshot as evidence of testing done for the new option in cdap sandbox.
Overall looks good.
...data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/RemoteValidationTask.java
Outdated
Show resolved
Hide resolved
...ap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/ValidationHandler.java
Outdated
Show resolved
Hide resolved
04bf38c
to
faa037d
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.
Please add unit test for MacroParser
for the newly added option.
...cdap-data-pipeline-base/src/test/java/io/cdap/cdap/datapipeline/DataPipelineServiceTest.java
Show resolved
Hide resolved
...ap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/ValidationHandler.java
Outdated
Show resolved
Hide resolved
...cdap-data-pipeline-base/src/test/java/io/cdap/cdap/datapipeline/DataPipelineServiceTest.java
Outdated
Show resolved
Hide resolved
...dap-etl-proto/src/main/java/io/cdap/cdap/etl/proto/v2/validation/StageValidationRequest.java
Show resolved
Hide resolved
...dap-etl-proto/src/main/java/io/cdap/cdap/etl/proto/v2/validation/StageValidationRequest.java
Show resolved
Hide resolved
6954d43
to
0c6c1f3
Compare
0c6c1f3
to
79ff250
Compare
...cdap-data-pipeline-base/src/test/java/io/cdap/cdap/datapipeline/DataPipelineServiceTest.java
Outdated
Show resolved
Hide resolved
...cdap-data-pipeline-base/src/test/java/io/cdap/cdap/datapipeline/DataPipelineServiceTest.java
Outdated
Show resolved
Hide resolved
...cdap-data-pipeline-base/src/test/java/io/cdap/cdap/datapipeline/DataPipelineServiceTest.java
Outdated
Show resolved
Hide resolved
79ff250
to
6cf161b
Compare
…throw error in case of invalid oauth macro.
6cf161b
to
1af2783
Compare
CDAP-21089 OAuthHandler should not return 5xx on 4xx errors to skip retries. Also added a doNotSkipInvalidMacroForFunctions in StageValidationRequest method to throw error in case provided macros are invalid.
can be used like
namespaces/system/apps/pipeline/services/studio/methods/v1/contexts/default/validations/stage?doNotSkipInvalidMacroForFunctions=oauthAccessToken,oauth
Before Adding doNotSkipInvalidMacroForFunctions, it was not throwing any error
After Adding doNotSkipInvalidMacroForFunctions, it was throwing below error