-
Notifications
You must be signed in to change notification settings - Fork 278
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
add permissions for index.json #1620
add permissions for index.json #1620
Conversation
Signed-off-by: Tianle Huang <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #1620 +/- ##
============================================
+ Coverage 94.32% 94.67% +0.35%
- Complexity 12 13 +1
============================================
Files 147 163 +16
Lines 3153 3437 +284
Branches 8 21 +13
============================================
+ Hits 2974 3254 +280
- Misses 172 180 +8
+ Partials 7 3 -4
Continue to review full report at Codecov.
|
send the functional part first. Unit test is in progress. |
The IBucket is interface which makes the unit test complicated. I first tried to provide a dummy implementation of it but ended up with a tedious piece of code.
After digging around, there are some solutions for mocking interfaces in TS. E.g https://medium.com/@vittorioguerriero/typescript-mock-for-real-b15147fddd92 A few of them point to a library https://github.com/Typescript-TDD/jest-ts-auto-mock Will check more and also see others' input before introducing this dependency. |
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'd prefer if the groovy changes from src/jenkins/BuildManifest.groovy and vars/uploadArtifacts.groovy in #1569 are also in this PR. It makes this more of a complete change and it removes those files from that PR.
I plan to make the groovy changes after merging which will be my next PR. The reason is that once this permission change is merged (and then |
Signed-off-by: Tianle Huang <[email protected]>
Link to main issue #1492 |
Signed-off-by: Tianle Huang [email protected]
Description
As #1569 grows, I want to commit by phases and also make it easy for me to test by using infra's Jenkins. Without this change, I have to test my Jenkins changes by uploading to
dist/index.json
.Thank @peternied for showing me the location of the change in tianleh#3
Post merge, I may need to perform
cdk deploy
(https://github.com/opensearch-project/opensearch-build/tree/main/deployment). Currently I do not have permission for performing such. I may need either 1) be granted such permission 2) have an infra team member with such permission to runcdk deploy
for me.Issues Resolved
Resolves #1601
Test
UT in progress.
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.