fix(pipelines): use display name for Asset actions when publishAssetsInParallel is false

[Tietew]

Issue # (if applicable)

Closes #34042.

Reason for this change

The action name in Assets stage has been changed from FileAssetN to the display name of the asset in #33844.

When pipeline's publishAssetsInParallel is false, the action name is also changed to the display name of the first asset in stages instead of FileAsset.
The action uploads all assets, therefore, its name should indicate "all assets" rather than the first asset.

Description of changes

Use a specific (stable) display name when singlePublisher is true (pipeline's publishAssetsInParallel is false), instead of stackAsset.displayName.

Describe any new or updated permissions being added

N/A

Description of how you validated changes

Added a unit test and a integ test.

Checklist

• My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.98%. Comparing base (994e952) to head (92c7ee5).

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #34049   +/-   ##
=======================================
  Coverage   83.98%   83.98%           
=======================================
  Files         120      120           
  Lines        6976     6976           
  Branches     1178     1178           
=======================================
  Hits         5859     5859           
  Misses       1005     1005           
  Partials      112      112           

Flag	Coverage Δ
suite.unit	83.98% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	∅ <ø> (∅)
packages/aws-cdk-lib/core	83.98% <ø> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[matboros]

I am not too fond of depending on the id for displayname, as a maintainer assumption would be that that is an implementation detail. What about:

Suggested change

	const displayName = this.singlePublisher ? id : stackAsset.displayName;
	const getDisplayNameForSinglePublishStep: (type: AssetType) => string = (type) => {
	switch(type) {
	case AssetType.FILE: return 'FileAsset(s)';
	case AssetType.DOCKER_IMAGE: return 'DockerAsset(s)';
	};
	}
	const displayName = this.singlePublisher ? getDisplayNameForSinglePublishStep(stackAsset.assetType) : stackAsset.displayName;

[Tietew]

Thanks for pointing out. But switch-case seems a bit long since id uses ?:...

[Tietew]

Unfortunately, FileAsset(s) was replaced by FileAsset_s_ :(

  CHANGED    pipelines/test/integ.newpipeline-single-publisher 3.847s
      Resources
[~] AWS::CodePipeline::Pipeline Pipeline9850B417
 └─ [~] Stages
     └─ @@ -121,7 +121,7 @@
        [ ]     "Name": "Synth_Output"
        [ ]   }
        [ ] ],
        [-] "Name": "FileAsset",
        [+] "Name": "FileAsset_s_",
        [ ] "RoleArn": {
        [ ]   "Fn::GetAtt": [
        [ ]     "PipelineCodeBuildActionRole226DB0CB",

[matboros]

Thanks for pointing out. But switch-case seems a bit long since id uses ?:...

That might be, but it improves maintainability, since if you add a 3rd asset type, the switch code will stop compiling, while the other will happily assume that the new assets are just DockerAsset, and likely be a bug, unless someone finds all such places.

[matboros]

Unfortunately, FileAsset(s) was replaced by FileAsset_s_ :(

  CHANGED    pipelines/test/integ.newpipeline-single-publisher 3.847s
      Resources
[~] AWS::CodePipeline::Pipeline Pipeline9850B417
 └─ [~] Stages
     └─ @@ -121,7 +121,7 @@
        [ ]     "Name": "Synth_Output"
        [ ]   }
        [ ] ],
        [-] "Name": "FileAsset",
        [+] "Name": "FileAsset_s_",
        [ ] "RoleArn": {
        [ ]   "Fn::GetAtt": [
        [ ]     "PipelineCodeBuildActionRole226DB0CB",

Noooooooo, that is so disappointing. So what do you think we should do? Use FileAsset or say FileAssets (plural)? Which one is less confusing for a user?
An alternative is to make it dependent on the number of assets (e.g FileAsset when only one asset is present, and FileAssets when multiple)? I assume the last one is the least confusing, but needs 2 extra tests.
What do you think?

[Tietew]

Use FileAsset or say FileAssets (plural)?
Which one is less confusing for a user?

I'm unsure because I'm a Japanese speaker... (Japanese has no plurals forms)

Before #33844, the name of action is the id -- FileAsset or DeployAsset (singular).
(That's the reason I originally used id.)

If it keeps as singular form, users won't experience any unintended changes around pipelines when upgrading cdk from 2.186.0 or earlier.

[matboros]

OK, lets just use plural, that is much more likely to be true.

PS. sorry for the slow answer, I was out for a bit.

[Tietew]

Ok. Fixed.

[Abogical]

To add to my review, PR title should be renamed to state what the fix is, it shouldn't just be a copy of the bug title.

Pull request has been modified.

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: 0e7d62e
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[matboros]

lgtm

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[github-actions]

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.