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

(pipelines): additionalinputs cannot mount deep directory #16936

Closed
lxop opened this issue Oct 13, 2021 · 4 comments · Fixed by #17074
Closed

(pipelines): additionalinputs cannot mount deep directory #16936

lxop opened this issue Oct 13, 2021 · 4 comments · Fixed by #17074
Assignees
Labels
@aws-cdk/aws-codepipeline Related to AWS CodePipeline @aws-cdk/pipelines CDK Pipelines library bug This issue is a bug. effort/small Small work item – less than a day of effort p1

Comments

@lxop
Copy link

lxop commented Oct 13, 2021

What is the problem?

As part of a CodePipeline, if I specify an additional input that should be placed somewhere within the primary input tree, that input appears within another directory inside the directory that it should be in.

As an example, with this configuration:

pipeline = pipelines.CodePipeline(
    [...]
    synth=pipelines.CodeBuildStep(
        "Synth",
        input=source,
        additional_inputs={"./web/dist": frontend_build},
    )
)

I would expect the files from frontend_build to appear inside web/dist/ in the tree. However, in fact they appear under web/dist/01/.

I have determined the cause, and it is here (this isn't the commit that added the bug, just a recent permalink):

    if (!['.', '..'].includes(path.dirname(input.directory))) {
      fragments.push(`mkdir -p -- "${input.directory}"`);
    }

That is: if the additional input should be placed anywhere other than one directory away from the build root (in my example it is two away), then that directory is created first. But now the subsequent behaviour of ln -s ... changes:

ln -s -- source dest

will place source at dest if it doesn't exist, but in dest if it does.

The solution is straightforward: just drop one directory from the mkdir call:

      fragments.push(`mkdir -p -- "${path.dirname(input.directory)}"`);

Reproduction Steps

A simple minimal reproducible bit of code is difficult to produce with pipelines due to the nature of the sources. Assuming you have a bootstrapped environment and a standard cdk.json file, put this into app.py, add any source entity that you have access to where noted, and deploy. The pipeline will fail in its Synth step because I'm not creating a cdk.out directory, but the log output of that step should show that file_from_src2 is in web/dist/01/ rather than web/dist/ where it should be.

from aws_cdk import (
    core as cdk,
    pipelines,
)


class PipelineStack(cdk.Stack):
    def __init__(self, scope: cdk.Construct, construct_id: str, **kwargs):
        super().__init__(scope, construct_id, **kwargs)
        source1 = pipelines.CodePipelineSource.<some source>
        source2 = pipelines.ShellStep(
            "src2",
            commands=[
                "touch file_from_src2",
            ],
            primary_output_directory=".",
        )
        pipelines.CodePipeline(
            self,
            "MyPipeline",
            synth=pipelines.ShellStep(
                "Synth",
                input=source1,
                additional_inputs={"web/dist": source2},
                commands=[
                    f"find web",
                ],
            ),
        )


pipeline_app = cdk.App()
PipelineStack(pipeline_app, "PipelineStack2")
pipeline_app.synth()

What did you expect to happen?

I expect the files from the additional inputs to appear directly in the directory that I have configured.

What actually happened?

The files from the additional inputs appear in a directory 01/ inside the directory that I have configured (I assume if more than one additional input is configured, subsequent ones will go to 02/, 03/, etc).

CDK CLI Version

1.126.0 (build f004e1a)

Framework Version

1.126

Node.js Version

v16.10.0

OS

Debian Linux

Language

Python

Language Version

3.8.12

Other information

See above for suggested fix.

@lxop lxop added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Oct 13, 2021
@github-actions github-actions bot added the @aws-cdk/aws-codepipeline Related to AWS CodePipeline label Oct 13, 2021
@skinny85 skinny85 added @aws-cdk/pipelines CDK Pipelines library and removed @aws-cdk/aws-codepipeline Related to AWS CodePipeline labels Oct 18, 2021
@skinny85 skinny85 assigned rix0rrr and unassigned skinny85 Oct 18, 2021
@rix0rrr
Copy link
Contributor

rix0rrr commented Oct 20, 2021

This feature was never intended to add files to an existing directory, just to conveniently "mount" the input somewhere in the directory space. If you want to merge directories together, the way to do that would be to add a cp command.

We're going to have to make it throw an error when you try to do that. Thanks for reporting.

@rix0rrr rix0rrr changed the title (pipelines): additional inputs can get an unexpected path component (pipelines): additional inputs should error when directory already exists Oct 20, 2021
@rix0rrr rix0rrr added effort/small Small work item – less than a day of effort and removed needs-triage This issue or PR still needs to be triaged. labels Oct 20, 2021
@rix0rrr rix0rrr removed their assignment Oct 20, 2021
@github-actions github-actions bot added the @aws-cdk/aws-codepipeline Related to AWS CodePipeline label Oct 20, 2021
@lxop
Copy link
Author

lxop commented Oct 20, 2021

Hi @rix0rrr , thanks for looking at this. I think you have misunderstood what is happening: I am not trying to put the inputs into a directory that already exists, rather the current logic causes the destination directory to exist if it is more than one level away from ./. For example, if I were to specify that an additional input should be mounted at ./newdir, everything would work fine, but if instead I were to specify that it should be mounted at ./newdir/this/definitely/does/not/exist, then the input would not be mounted there, but at ./newdir/this/definitely/does/not/exist/01.

The fix is quite trivial, as shown in the main description.

@rix0rrr
Copy link
Contributor

rix0rrr commented Oct 20, 2021

Oh I see what you're saying. You're right, I misread.

@rix0rrr rix0rrr changed the title (pipelines): additional inputs should error when directory already exists (pipelines): additionalinputs cannot mount deep directory Oct 20, 2021
rix0rrr added a commit that referenced this issue Oct 20, 2021
If the directory is nested deeper than one level underneath `.` or `..`,
the wrong directory gets created.

Also add in protection against the directory already existing, in which
case the same behavior would happen.

Fixes #16936.
@mergify mergify bot closed this as completed in #17074 Oct 20, 2021
mergify bot pushed a commit that referenced this issue Oct 20, 2021
If the directory is nested deeper than one level underneath `.` or `..`,
the wrong directory gets created.

Also add in protection against the directory already existing, in which
case the same behavior would happen.

Fixes #16936.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️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.

TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this issue Feb 21, 2022
If the directory is nested deeper than one level underneath `.` or `..`,
the wrong directory gets created.

Also add in protection against the directory already existing, in which
case the same behavior would happen.

Fixes aws#16936.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-codepipeline Related to AWS CodePipeline @aws-cdk/pipelines CDK Pipelines library bug This issue is a bug. effort/small Small work item – less than a day of effort p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants