-
Notifications
You must be signed in to change notification settings - Fork 428
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(cicd): allow CodeCommit as pipeline source #1808
Conversation
…o); start adding conditionals to cfn template
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.
This is awesome! Thank you so much. Can we also make sure that integ test works well for this change?
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.
🚀
Configuration: | ||
# TODO: #273 For now this set of configurations is GitHub-specific, | ||
# change it to be more generic | ||
Owner: {{$.Source.Owner}} | ||
Repo: {{$.Source.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.
Do these fields come from deploy.GithubSource
or manifest.GithubProperties
, or somewhere else? In those structs the field is RepositoryURL
, not Repository
--is that correct?
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.
Good question-- this is confusing.
Repository is the method (func (s *GitHubSource) Repository() (string, error)
) in the deploy package that returns the repo of a GitHubSource. For this field, we want just the repo name, not the whole url.
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.
Interesting, so Go will call the method named there. I talked about this in another comment where I think we can combine a base struct with a public ProviderName method and generator function. It's not super necessary though so if you think it's too much bother feel free to 🚀
ProviderName string | ||
// GitHubSource defines the (GH) source of the artifacts to be built and deployed. | ||
type GitHubSource struct { | ||
ProviderName string |
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.
Is this the same struct which determines the values that get injected into CFN? Is that why we need the ProviderName
struct member?
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.
Yeah--I had taken it out because it seemed redundant, but this seemed to be the most straightforward way to determine which chunk of the yaml should get applied to the template.
I may change this when I introduce other sources and partials. Let me know if you have ideas!
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.
We could create a base pipelineSource
struct which has a ProviderName()
method and include that as a member of both GithubSource
and CodeCommitSource
, as something like
type pipelineSource struct {
providerName string
}
func (s *pipelineSource) ProviderName() string {
return s.providerName
}
type GithubSource struct {
*pipelineSource
// other GH specific members
}
func NewGithubSource(url, branch, secretID string) GithubSource {
return GithubSource{
pipelineSource: &pipelineSource{
providerName: manifest.GithubProviderName
},
// other members
}
}
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.
This is cool! I implemented it locally but didn't commit because it may overcomplicate things at this point (unless there's a benefit to the cfn template calling a method rather than a struct field). I'll keep this in mind, though, if we have more functionality to add to this set-up with more sources.
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.
There's no particular benefit except
a) limiting code redeclaration in the various pipeline providers
b) making it harder to incorrectly initialize GithubSource
or CodeCommitSource
with the wrong provider name.
If the struct becomes more complicated then I think it would be worth it to move to the constructor pattern above but for the time being it's totally fine to leave as is.
Co-authored-by: Penghao He <[email protected]>
This PR enables CodeCommit as a pipeline source through execution  (while still allowing GitHub as a source).  Resolves aws#781 Related aws#1339 By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
This PR enables CodeCommit as a pipeline source through execution

(while still allowing GitHub as a source).

Resolves #781
Related #1339
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.