(CodeBuild Report Group): Defining the report type in CDK

[CodeEngineTechnology]

When you create a new Report Group you should be able to select the report type (e,g,. Coverage)

There is no way to change the report type from Test to Coverage in props
new ReportGroup(this.scope, 'test-coverage', {reportGroupName: 'test-coverage'});

Use Case

Proposed Solution

Other

• 👋 I may be able to implement this feature request
• ⚠️ This feature might incur a breaking change

---

This is a 🚀 Feature Request

[skinny85]

Thanks for the feature request @CodeEngineTechnology!

[kostiantyn-lab]

Is there a chance to solve this?

[skinny85]

@kostiantyn-lab PRs are always welcome!

In the meantime, you can probably use escape hatches to work around this problem.

[bilalq]

The escape hatch approach seems to have some limitations. When I use it like so:

const unitTestCoverageGroup = new ReportGroup(this, 'CoverageReportGroup', {
  removalPolicy: cdk.RemovalPolicy.RETAIN
})
const cfnUnitTestCoverageGroup = unitTestCoverageGroup.node.defaultChild as CfnReportGroup
cfnUnitTestCoverageGroup.type = 'CODE_COVERAGE'

// ...

 unitTestCoverageGroup.grantWrite(buildStep.grantPrincipal)

The grantWrite seems to have no effect. My builds fail with errors like:

Error in UPLOAD_ARTIFACTS phase: [arn:aws:codebuild:us-east-2:123456789012:report-group/CoverageReportGroupE23151CF-SZhAqquaRliT: 
[error processing report: [AccessDeniedException: User: arn:aws:sts::123456789012:assumed-role/ExperimentPipeline-ExperimentsPipelineBetaExperime-1450ZCD21X2G3/AWSCodeBuild-9efbe70f-0ccf-43dc-8930-d752ff543286 is not authorized to perform: codebuild:BatchPutCodeCoverages on resource: arn:aws:codebuild:us-east-2:123456789012:report-group/CoverageReportGroupE23151CF-SZhAqquaRliT
--
1000 | status code: 400, request id: 88054346-af0c-46ea-86ca-9ef2e2626654 InvalidInputException: Report status is invalid]]]

EDIT:

I see the issue. The grantWrite implementation is very much tied to the APIs for writing test reports. Coverage reports need different permissions. There needs to be an explicit grant for codebuild:BatchPutCodeCoverages.

This problem is made worse by the fact that the IAM docs for CodeBuild don't mention the existence of this operation at all.

https://docs.aws.amazon.com/codebuild/latest/userguide/auth-and-access-control-permissions-reference.html

The workaround ends up being to have the following:

import { BuildSpec, ReportGroup, CfnReportGroup } from '@aws-cdk/aws-codebuild'
import * as cdk from '@aws-cdk/core'
import * as pipelines from '@aws-cdk/pipelines'
import * as iam from '@aws-cdk/aws-iam'

interface SomePipelineProps extends cdk.StackProps {
  // ...
}

export class SomePipeline extends cdk.Stack {
  constructor(scope: cdk.Construct, id: string, props: SomePipelineProps) {
    super(scope, id, props)

    const repo = 'Your Repo here'
    const branch = 'Your branch here'
    const codeStarConnection = pipelines.CodePipelineSource.connection(repo, branch, {
      connectionArn: 'Your CodeStar connection ARN here',
      triggerOnPush: true
    })

    const jestReportGroup = new ReportGroup(this, 'JestReportGroup', {})
    const unitTestCoverageGroup = new ReportGroup(this, 'CoverageReportGroup', {})
    const cfnUnitTestCoverageGroup = unitTestCoverageGroup.node.defaultChild as CfnReportGroup
    cfnUnitTestCoverageGroup.type = 'CODE_COVERAGE'

    const customSynthStep = new pipelines.CodeBuildStep('ExperimentalSynth', {
      input: codeStarConnection,
      env: {
        CI: 'true'
      },
      installCommands: ['npm ci'],
      commands: ['npm run build', 'npm test', 'npx cdk synth'],
      partialBuildSpec: BuildSpec.fromObject({
        version: '0.2',
        // Make sure your jest config outputs to locations that match what's here
        reports: {
          [jestReportGroup.reportGroupArn]: {
            files: ['jest-results.xml'],
            'file-format': 'JUNITXML',
            'base-directory': 'build/reports/unit-test'
          },
          [unitTestCoverageGroup.reportGroupArn]: {
            files: ['clover.xml'],
            'file-format': 'CLOVERXML',
            'base-directory': 'build/reports/coverage'
          }
        }
      }),
      primaryOutputDirectory: 'cdk.out'
    })

    const pipeline = new pipelines.CodePipeline(this, 'SomePipeline', {
      pipelineName: 'SomePipeline',
      synth: customSynthStep
    })

    // Do whatever else you want on your pipeline
    // ...

    // Granting access here has to happen after calling buildPipeline so the CloudAssembly references resolve
    // All of this is only necessary becasue of limitations discussed in https://github.com/aws/aws-cdk/issues/10464
    pipeline.buildPipeline()
    jestReportGroup.grantWrite(customSynthStep.grantPrincipal)
    unitTestCoverageGroup.grantWrite(customSynthStep.grantPrincipal)
    // This next grant is necessary because the grantWrite command won't add the permission needed for coverage reports
    // https://github.com/aws/aws-cdk/issues/14279
    iam.Grant.addToPrincipal({
      grantee: customSynthStep.grantPrincipal,
      actions: ['codebuild:BatchPutCodeCoverages'],
      resourceArns: [unitTestCoverageGroup.reportGroupArn]
    })
  }
}

[skinny85]

Thanks for posting that @bilalq! Will be super useful when implementing this feature.

[bilalq]

I'm considering working on a PR to address both this and #10464.

My thoughts on the approach:

• Create an enum for ReportGroupType that is either "TEST" or "CODE_COVERAGE".
• Update the ReportGroup construct to accept a property called type with a type of ReportGroupType and set it on the underlying CFN.
• Update the grantWrite implementation to grant codebuild:BatchPutTestCases for ReportGroupType.TEST and codebuild:BatchPutCodeCoverages for ReportGroupType.CODE_COVERAGE.
• Update CodeBuildStep from @aws-cdk/pipelines to accept a reports property as a first-class field. It should accept a property for the ReportGroup, as well as the other things you'd usually put in a CodeBuildSpec such as files, fileFormat (which I'll also create an enum for), and baseDirectory. This should also take care of doing whatever permission grants are necessary.

Would you have objections/concerns with any of the above @skinny85?

[skinny85]

No, that sounds great @bilalq!

The only possible comment I have is to think whether it's better to introduce a property to ReportGroupProps, or whether it's better to introduce a separate class (something like CoverageReportGroup).

I would lean towards having a separate CoverageReportGroup class if there either already are, or we expect there might be in the future, properties that can only be set for a ReportGroup if its type is 'CODE_COVERAGE' (because, if we have a single class, we would have to introduce runtime validation in that case, which is always worse that compile-time validation). If there are no properties like that today, and we don't anticipate ones like that will be created in the future, I'm fine with having a single ReportGroup class.

Something to consider!

[bilalq]

I don't really feel strongly either way. The only reason I suggested using an adjustment on the existing class was that it mirrors the underlying CFN resource more that way.

I suppose a better implementation here would be to have ReportGroup be an interface that TestReportGroup and CoverageReportGroup could both implement, but I guess it's too late to do that without impacting backwards compatibility.

Another option would be to make ReportGroup take a property like I suggested earlier and have two subclasses while preserving the existing behavior as the default. Thoughts?

Today, they don't have different properties, but they do have different valid combinations with the buildspec property on the related CodeBuild project object. The reports/<report-group>/file-format property of buildspecs can have test reports can be one of
CUCUMBERJSON | JUNITXML | NUNITXML | NUNIT3XML | TESTNGXML | VISUALSTUDIOTRX, while coverage reports must have it be one of CLOVERXML | COBERTURAXML | JACOCOXML | SIMPLECOV.

It's worth calling out that buildspecs can be imported from files, so compile time guarantees can't be made without additional runtime checking.

[skinny85]

Another option would be to make ReportGroup take a property like I suggested earlier and have two subclasses while preserving the existing behavior as the default. Thoughts?

Yep, that actually sounds pretty fantastic!

[github-actions]

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.