-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Conversation
Co-authored-by: Ben Chaimberg <youppi3@gmail.com>
@@ -0,0 +1 @@ | |||
# Test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this accidental?
There was a problem hiding this comment.
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 🙂.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am using it to do the integration test. See https://github.com/aws/aws-cdk/pull/17968/files#diff-026923c68996b31cf2751e6c0cebe38da74216f6f31841614b8a5604f6fe694aR82
There was a problem hiding this comment.
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
😃.
There was a problem hiding this comment.
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 :)
There was a problem hiding this 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 |
There was a problem hiding this comment.
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 🙂.
packages/@aws-cdk/aws-codecommit/test/integ.codecommit-code-asset.ts
Outdated
Show resolved
Hide resolved
path: path.join(__dirname, 'directory/'), | ||
}); | ||
|
||
const repo = new Repository(this, 'Repository', { |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 🙂.
Pull request has been modified.
There was a problem hiding this 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', { |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
packages/@aws-cdk/aws-codecommit/test/integ.codecommit-code-asset-zip.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-codecommit/test/integ.codecommit-code-asset.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-codecommit/test/integ.codecommit-code-asset-zip.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-codecommit/test/integ.codecommit-code-asset.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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>
Pull request has been modified.
There was a problem hiding this 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!
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 CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
…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*
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