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

add permissions for index.json #1620

Merged
merged 1 commit into from
Feb 14, 2022

Conversation

tianleh
Copy link
Member

@tianleh tianleh commented Feb 14, 2022

Signed-off-by: Tianle Huang tianleh@amazon.com

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 run cdk deploy for me.

Issues Resolved

Resolves #1601

Test

UT in progress.

Check List

  • Commits are signed per the DCO using --signoff

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.

Signed-off-by: Tianle Huang <tianleh@amazon.com>
@tianleh tianleh requested a review from a team as a code owner February 14, 2022 08:10
@codecov-commenter
Copy link

codecov-commenter commented Feb 14, 2022

Codecov Report

Merging #1620 (ec8909c) into main (cf6314b) will increase coverage by 0.35%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             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     
Impacted Files Coverage Δ
deployment/lib/identities.ts 100.00% <100.00%> (ø)
src/run_bwc_test.py 86.66% <0.00%> (-6.67%) ⬇️
src/manifests/manifest.py 91.56% <0.00%> (-3.62%) ⬇️
src/test_workflow/bwc_test/bwc_test_suite.py 92.45% <0.00%> (-0.89%) ⬇️
src/jenkins/BuildManifest.groovy 91.42% <0.00%> (-0.58%) ⬇️
src/system/execute.py 100.00% <0.00%> (ø)
src/paths/output_dir.py 100.00% <0.00%> (ø)
src/paths/tree_walker.py 100.00% <0.00%> (ø)
src/ci_workflow/ci_args.py 100.00% <0.00%> (ø)
src/ci_workflow/ci_target.py 100.00% <0.00%> (ø)
... and 46 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cf6314b...ec8909c. Read the comment docs.

@tianleh
Copy link
Member Author

tianleh commented Feb 14, 2022

send the functional part first. Unit test is in progress.

@tianleh
Copy link
Member Author

tianleh commented Feb 14, 2022

import { IBucket } from '@aws-cdk/aws-s3';

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.

import { Grant, IGrantable } from "@aws-cdk/aws-iam";
import { BucketPolicy, IBucket } from "@aws-cdk/aws-s3";

class TestIBucket implements IBucket{
    bucketArn: string;
    bucketName: string;
    bucketWebsiteUrl: string;
    bucketWebsiteDomainName: string;
    bucketDomainName: string;
    bucketDualStackDomainName: string;
    bucketRegionalDomainName: string;
    isWebsite?: boolean | undefined;
    encryptionKey?: IKey | undefined;
    policy?: BucketPolicy | undefined;
    addToResourcePolicy(permission: PolicyStatement): AddToResourcePolicyResult {
        throw new Error('Method not implemented.');
    }
    urlForObject(key?: string): string {
        throw new Error('Method not implemented.');
    }
    transferAccelerationUrlForObject(key?: string, options?: TransferAccelerationUrlOptions): string {
        throw new Error('Method not implemented.');
    }
    virtualHostedUrlForObject(key?: string, options?: VirtualHostedStyleUrlOptions): string {
        throw new Error('Method not implemented.');
    }
    s3UrlForObject(key?: string): string {
        throw new Error('Method not implemented.');
    }
    arnForObjects(keyPattern: string): string {
        throw new Error('Method not implemented.');
    }
    grantRead(identity: IGrantable, objectsKeyPattern?: any): Grant {
        throw new Error('Method not implemented.');
    }
    grantWrite(identity: IGrantable, objectsKeyPattern?: any): Grant {
        throw new Error('Method not implemented.');
    }
    grantPut(identity: IGrantable, objectsKeyPattern?: any): Grant {
        throw new Error('Method not implemented.');
    }
    grantPutAcl(identity: IGrantable, objectsKeyPattern?: string): Grant {
        throw new Error('Method not implemented.');
    }
    grantDelete(identity: IGrantable, objectsKeyPattern?: any): Grant {
        throw new Error('Method not implemented.');
    }
    grantReadWrite(identity: IGrantable, objectsKeyPattern?: any): Grant {
        throw new Error('Method not implemented.');
    }
    grantPublicAccess(keyPrefix?: string, ...allowedActions: string[]): Grant {
        throw new Error('Method not implemented.');
    }
    onCloudTrailEvent(id: string, options?: OnCloudTrailBucketEventOptions): Rule {
        throw new Error('Method not implemented.');
    }
    onCloudTrailPutObject(id: string, options?: OnCloudTrailBucketEventOptions): Rule {
        throw new Error('Method not implemented.');
    }
    onCloudTrailWriteObject(id: string, options?: OnCloudTrailBucketEventOptions): Rule {
        throw new Error('Method not implemented.');
    }
    addEventNotification(event: EventType, dest: IBucketNotificationDestination, ...filters: NotificationKeyFilter[]): void {
        throw new Error('Method not implemented.');
    }
    addObjectCreatedNotification(dest: IBucketNotificationDestination, ...filters: NotificationKeyFilter[]): void {
        throw new Error('Method not implemented.');
    }
    addObjectRemovedNotification(dest: IBucketNotificationDestination, ...filters: NotificationKeyFilter[]): void {
        throw new Error('Method not implemented.');
    }
    stack: Stack;
    env: ResourceEnvironment;
    applyRemovalPolicy(policy: RemovalPolicy): void {
        throw new Error('Method not implemented.');
    }
    node: ConstructNode;

}

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.

Copy link
Member

@peternied peternied left a 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.

@peternied
Copy link
Member

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.

@tianleh Would you like to make this change now or after merging?

@tianleh
Copy link
Member Author

tianleh commented Feb 14, 2022

@tianleh Would you like to make this change now or after merging?

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 cdk deployed), my Jenkins job in the "Playground" will be able to actually upload the index.json file instead of the current workaround location dist/index.json. It will give me more confidence about the groovy changes.

@peternied peternied merged commit bddbc06 into opensearch-project:main Feb 14, 2022
peterzhuamazon pushed a commit to peterzhuamazon/opensearch-build that referenced this pull request Feb 16, 2022
Signed-off-by: Tianle Huang <tianleh@amazon.com>
@tianleh
Copy link
Member Author

tianleh commented Mar 2, 2022

Link to main issue #1492

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow S3 uploading of index.yml file besides builds/dist
4 participants