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(codecommit): allow initializing a Repository with contents #17968

Merged
merged 19 commits into from
Dec 14, 2021
Merged

feat(codecommit): allow initializing a Repository with contents #17968

merged 19 commits into from
Dec 14, 2021

Conversation

LukvonStrom
Copy link
Contributor

@LukvonStrom LukvonStrom commented Dec 11, 2021

This repo introduces new properties to the constructor signature of the codecommit L2 Repository.
It allows users to upload code when creating a codecommit repository by leveraging the aws-s3-assets lib.
The user is able to upload whole directories at the moment.

The behaviour has a unit tests, and an integration test, which is passing (verified manually as well).

Closes #17967, provides a possible fix to #16958


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

@gitpod-io
Copy link

gitpod-io bot commented Dec 11, 2021

@github-actions github-actions bot added the @aws-cdk/aws-codecommit Related to AWS CodeCommit label Dec 11, 2021
@@ -0,0 +1 @@
# Test
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this accidental?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, let's kill this 🙂.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's call this something like test-file.txt then. Not README.md 😃.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@skinny85 it is a README to easily verify its existence when clicking on the codecommit repo, but I can rename it :)

Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @LukvonStrom! It's a great start, but I feel like we need to iterate on the API a little bit more before merging this in.

@@ -0,0 +1 @@
# Test
Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, let's kill this 🙂.

path: path.join(__dirname, 'directory/'),
});

const repo = new Repository(this, 'Repository', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Code here should use the codecommit prefix (see examples above).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we should make it consistent with the contributing guide:
https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md#recommendations
Types from the documented module should be un-qualified

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we should fix that - that's not the convention we use today (the examples should show what the customer of the library will write, so it should be qualified). Thanks for pointing that out!

And let's change it for these new examples 🙂.

@mergify mergify bot dismissed skinny85’s stale review December 13, 2021 22:42

Pull request has been modified.

Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @LukvonStrom! A few comments before we merge this one in 🙂.

path: path.join(__dirname, 'directory/'),
});

const repo = new Repository(this, 'Repository', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we should fix that - that's not the convention we use today (the examples should show what the customer of the library will write, so it should be qualified). Thanks for pointing that out!

And let's change it for these new examples 🙂.

```ts
const repo = new Repository(this, 'Repository', {
repositoryName: 'MyRepositoryName',
code: Code.fromDirectory(path.join(__dirname, 'directory/'), 'develop'), // optional property, branch parameter can be omitted
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if code / Code is too specific a word here. What do you think about renaming the property and the class to contents / Contents? Or maybe even contents for the property, and RepositoryContents for the class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As Git is a version control system for primarily code, I would leave it on code for now, if that's ok with you?

@LukvonStrom LukvonStrom requested a review from skinny85 December 14, 2021 10:36
Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

Looks great @LukvonStrom! Just a few tiny last comments, mainly around docs, before we merge this in!

Co-authored-by: Adam Ruka <adamruka85@gmail.com>
@mergify mergify bot dismissed skinny85’s stale review December 14, 2021 21:19

Pull request has been modified.

@LukvonStrom LukvonStrom requested a review from skinny85 December 14, 2021 21:38
Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for the all work on this @LukvonStrom!

@skinny85 skinny85 changed the title feat(@aws-cdk/aws-codecommit): add code initialization for codecommit repository feat(codecommit): allow initializing a Repository with contents Dec 14, 2021
@mergify mergify bot merged commit 54b6cc6 into aws:master Dec 14, 2021
@mergify
Copy link
Contributor

mergify bot commented Dec 14, 2021

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 8ee793f
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
…17968)

This repo introduces new properties to the constructor signature of the codecommit L2 Repository.
It allows users to upload code when creating a codecommit repository by leveraging the aws-s3-assets lib.
The user is able to upload whole directories at the moment.

The behaviour has a unit tests, and an integration test, which is passing (verified manually as well).

Closes aws#17967, provides a possible fix to aws#16958

----

*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-codecommit Related to AWS CodeCommit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(@aws-cdk/aws-codecommit): Include CodeCommit Repo initialization via S3 in L2 construct
5 participants