Skip to content

Conversation

@aniketkb2014
Copy link
Contributor

  1. Adding the codepipeline source to choose from external version control system ( GitHub),
    a. Using codeconnection to configure setup for github

  2. Updating the iam policy required for tagging the resources

production?: Account;

// Getter methods for controlled access
getToolchain(): Account | undefined { return this.toolchain; }
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this is not idiomatic Typescript. See example

class foo {
    private _bar: boolean = false;
    get bar(): boolean {
        return this._bar;
    }
    set bar(value: boolean) {
        this._bar = value;
    }
}```

getProduction(): Account | undefined { return this.production; }

// Setter method with validation
private setAccount(type: 'toolchain' | 'beta' | 'gamma' | 'production', account: unknown): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

type needs to be a enum

}


export class ExternalSource extends Construct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this need to be it's own Construct Couldn't we extend CodePipelineSource or create a factory class instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We considered the external source to be a separate construct since it was experiencing different connectivity through codeconnections.

Copy link
Contributor

@davidhessler davidhessler Apr 7, 2025

Choose a reason for hiding this comment

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

By that logic, this should be a class, but I'm not seeing the benefit of a Construct. Particularly because in many cases the CodeConnection is global and/or created outside the scope of a specific CDK Construct (e.g. Stack). It is worth noting that CodePipelineSource itself is a class but does not Construct. Said simply, not all classes need to be a Construct.

What I believe the intent of this code here is to be a factory class that creates pre-configured instances of CodePipelineSource based upon a simplified set of parameters (e.g. ExternalSourceProps).

If that's the case then this class should look more the following

export class CodePipelineSourceFactory{
  static createCodePipelineSource(pipelineStack: PipelineStack){
    const providerType = pipelineStack.node.tryGetContext("providerType")==undefined?"codecommit":pipelineStack.node.tryGetContext("providerType");

    switch(providerType){
      case 'codecommit':
        const appName = pipelineStack.node.tryGetContext('appName')
        if(!appName){
          throw new Error('appName is required')
        }
        // CodeCommitSource is an instance of Construct
        return new CodeCommitSource(pipelineStack, 'Source', {repositoryName: appName})
      default:
        const repoParameters = {
          "owner": pipelineStack.node.tryGetContext('owner'),
          "repositoryName": pipelineStack.node.tryGetContext('repositoryName'),
          "trunkBranchName": pipelineStack.node.tryGetContext('trunkBranchName'),
          "connectionArn": pipelineStack.node.tryGetContext('connectionArn'),
        };

        // CodePipelineSource is not an instance of Construct
        return CodePipelineSource.connection(
          `${repoParameters.owner}/${repoParameters.repositoryName}`,
          repoParameters.trunkBranchName ?? 'main',
          { connectionArn: repoParameters.connectionArn }
        )
    }
}

@davidhessler davidhessler merged commit 40ca406 into aws-samples:main Apr 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants