feat: allow inclusion of image copy to pipeline

[Hi-Fi]

Fixes #201

[Hi-Fi]

@wchaws how this sounds?

I think this require more tests and probably some documentation, but just initial thought wanted before polishing.

[Hi-Fi]

Ah, currently this seems to be working only with CodePipeline, not with Pipelines. I'll check today that construct would be usable with both.

And now realized that also Lambda need to be a bit different on handler side, as events and returns are different.

[mrgrain]

I like this idea, but it shouldn't be added to the existing construct. Instead we will need a an ECRDeploymentAction and ECRDeploymentStep respectively that can be constructed explicitly. This should still allow re-use of the same functionality.

Additionally I am not sure if pipeline actions/steps should share the same singleton function with the custom resource. That seems undesirable to me. The CR is a means to end to achieve the IaC declaration of "I need a copy of this image in that place", the fact it creates a lambda function is an implementation detail.

However as a Pipeline Action/Step to me this really becomes what the IaC declaring: A function that can be used to copy images from A to B. I'm not even sure if these should be a singleton at all.

[Hi-Fi]

I'll try to check this. We have all this time used custom version of pipeline inclusion, and it has worked in a very nice way.

[Hi-Fi]

I like this idea, but it shouldn't be added to the existing construct. Instead we will need a an ECRDeploymentAction and ECRDeploymentStep respectively that can be constructed explicitly. This should still allow re-use of the same functionality.

Additionally I am not sure if pipeline actions/steps should share the same singleton function with the custom resource. That seems undesirable to me. The CR is a means to end to achieve the IaC declaration of "I need a copy of this image in that place", the fact it creates a lambda function is an implementation detail.

However as a Pipeline Action/Step to me this really becomes what the IaC declaring: A function that can be used to copy images from A to B. I'm not even sure if these should be a singleton at all.

Made some modifications. Also some refactoring with Lamdba to allow simpler handling of both cases with same golang code.

[mrgrain]

Appreciate the update! Could you please keep the refactoring and functional changes separate? It just makes it hard to review. I can't even tell which refactoring are to support the feature here and which are "just because", e.g. it's not at all clear why this requires a change the the golang code.

[Hi-Fi]

Appreciate the update! Could you please keep the refactoring and functional changes separate? It just makes it hard to review. I can't even tell which refactoring are to support the feature here and which are "just because", e.g. it's not at all clear why this requires a change the the golang code.

I think only refactoring here is to move things out from index.ts for clarity. I can revert so that index.ts has all things that are not needed also on other options, but it feels non-optimal solution.

Lambda needs changes as handler currently handles only event from CloudFormation. When step is added to CodePipeline, it has to handle different event, and also return the execution information to the pipeline.

Refactorings could be separated to another commit before adding new functionality, but I don't see that helping the review as "why" might be hard to see with that commit alone.

[mrgrain]

revert pls

[mrgrain]

revert or explain why this is needed

[Hi-Fi]

Go mod tidy seemed to change this automatically. Build uses public.ecr.aws/sam/build-go1.x:latest which has go 1.22.1, but have to check if this works as should with Go 1.15.

[Hi-Fi]

Seems that aws-sdk-go-v2 requires (according https://github.com/aws/aws-sdk-go-v2) Go 1.21. Not sure how those earlier ones have worked, or then this is always built with newer version than mentioned to be requirement on go.mod.

[mrgrain]

Okay, you can probably guess my request to pull this update out into a separate PR 🙈

Sorry that this issue is bringing up so many maintenance tasks.

[Hi-Fi]

OK, running those commands to check go.mod with older golang, as newer ones would update that automatically including also "toolchain" version there.

[mrgrain]

Refactorings could be separated to another commit before adding new functionality, but I don't see that helping the review as "why" might be hard to see with that commit alone.

It helps someone like me who is not super familiar with golang.

[Hi-Fi]

Refactorings could be separated to another commit before adding new functionality, but I don't see that helping the review as "why" might be hard to see with that commit alone.

It helps someone like me who is not super familiar with golang.

For Golang changes there's not really refactoring due to refactoring (except extra struct), but to prevent to have 2 handles with 80% same code (parsing of inputs and copying image). It can be done with more copy-paste way as inputs at the end are same

[mrgrain]

I think only refactoring h ere is to move things out from index.ts for clarity. I can revert so that index.ts has all things that are not needed also on other options, but it feels non-optimal solution.

I appreciate your concern. Whether options include unnecessary options seems completely unrelated to the setup of the project in a single file vs multiple files. Definitely open to that refactor, but make it as a separate PR please.

[mrgrain]

These seems like a refactor

[Hi-Fi]

True, modified the code different way that I would not like, but increasing technical debt then :)

[Hi-Fi]

Now I think all functions/interfaces that are moved from file to file are done to get ECRDeploymentStep done without exhaustive duplication of existing ones.

Refactored version was tested also within actual CodePipeline, this version without those refactoring isn't (yet).

[Hi-Fi]

I think only refactoring h ere is to move things out from index.ts for clarity. I can revert so that index.ts has all things that are not needed also on other options, but it feels non-optimal solution.

I appreciate your concern. Whether options include unnecessary options seems completely unrelated to the setup of the project in a single file vs multiple files. Definitely open to that refactor, but make it as a separate PR please.

So would be be better to include the new functionality to index.ts, and just put also all new (helper) functions there? And make that separation of files completely on other PR? Or is the current way of adding new files already for new functionalities (and helpers) OK?