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

feat(codepipeline): final form of the CodeBuild Pipeline action #2716

Merged
merged 1 commit into from
Jun 17, 2019

Conversation

skinny85
Copy link
Contributor

@skinny85 skinny85 commented Jun 1, 2019

BREAKING CHANGE: the output and extraOutputs properties of the CodeBuildAction were merged into one property outputs.


Pull Request Checklist

  • Testing
    • Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
    • CLI change?: coordinate update of integration tests with team
    • cdk-init template change?: coordinated update of integration tests with team
  • Docs
    • jsdocs: All public APIs documented
    • README: README and/or documentation topic updated
    • Design: For significant features, design document added to design folder
  • Title and Description
    • Change type: title prefixed with fix, feat and module name in parens, which will appear in changelog
    • Title: use lower-case and doesn't end with a period
    • Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
    • Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
  • Sensitive Modules (requires 2 PR approvers)
    • IAM Policy Document (in @aws-cdk/aws-iam)
    • EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
    • Grant APIs (only if not based on official documentation with a reference)

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

@skinny85 skinny85 requested a review from RomainMuller June 1, 2019 01:12
@skinny85 skinny85 requested a review from a team as a code owner June 1, 2019 01:12
@eladb
Copy link
Contributor

eladb commented Jun 2, 2019

I am failing to understand why we continuously make this API less ergonomic. This change goes against our design tenet to optimize for the common case. I’d argue that 99% of pipeline actions would have a single output. Can you please provide the motivation for this change?

@rix0rrr
Copy link
Contributor

rix0rrr commented Jun 3, 2019

When given the choice between:

interface {
  things: string[];
}

And:

interface {
  thing: string;

  moreThings?: string[];
}

I would also prefer the first one. Why would you purpose introduce that kind of shape irregularity on purpose?

For one thing, the convenience argument assumes it will always be a human supplying the input values, and they will trivially be able to pick the right argument.

At the same time, it makes it harder on any kind of automated or higher-level code, which for example might read the list of artifacts from a JSON file, or gets it from a service call (to name something)[1].

I made the same mistake with CompositePrincipal, and immediately a user complained because they had to jump through hoops to add a higher-level abstraction over it: #2327

Given that the cost is having to type an additional [ ], I don't think it's too bad.

[1] Pre-emptive response to "you can just pick off the first element and pass it into another field": yes, but you're introducing more busywork and chance of error. In a different but related typical programming example, you can also return null instead of an empty list but now you're making other people type ifs everywhere in order to deal with your special case that didn't need to be a special case. My opinion is that every design decision that leads to the addition of ifs that didn't need to be there, is wrong, since it increases chances of human fuck-up.

Yes, that means I will take element.setVisible(true|false) over element.show() and element.hide(), and that's a hill I'm willing to die on :P.

@eladb
Copy link
Contributor

eladb commented Jun 3, 2019

it’s all about API ergonomics. If in 99% of the cases actions will have a single output, having to pass in an array is less comfortable. In some languages (ahm java) it’s even annoying. We should constantly think about the common case and optimize for it.

If this was the only case, then okay, but I feel we have been continuously making our pipelines API less ergonomic (artifacts, etc) and I wanted to call out that this is not the right direction.

The CDK is all about the developer experience. More verbosity and ceremony creates friction and redundant boilerplate for customers, and we should avoid them as much as possible.

I am curious what is the motivation behind this change.

Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

I've given it some thought and I am fine with this. I feel we will need to build some L3s for pipelines anyway, and we'll make all our ergonomics dreams come true at that layer.

BREAKING CHANGE: the output and extraOutputs properties of the CodeBuildAction were merged into one property, outputs.
@skinny85 skinny85 force-pushed the feature/codebuild-pipeline-action branch from 658b1d0 to 31a347a Compare June 17, 2019 17:28
@skinny85
Copy link
Contributor Author

Rebased to solve conflicts.

@skinny85 skinny85 merged commit c10fc9a into aws:master Jun 17, 2019
@skinny85 skinny85 deleted the feature/codebuild-pipeline-action branch June 17, 2019 18:25
@NGL321 NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants