-
Notifications
You must be signed in to change notification settings - Fork 133
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
Retrieve git revision from CodePipeline #109
Retrieve git revision from CodePipeline #109
Conversation
a5f5033
to
b8b3314
Compare
b8b3314
to
e0f8293
Compare
@cplee - Should be ready for review. I don't have a test (yet). I am not sure if you would rather wait on one before merge or not. |
common/pipeline.go
Outdated
@@ -47,3 +48,26 @@ func (cplMgr *codePipelineManager) ListState(pipelineName string) ([]*codepipeli | |||
|
|||
return output.StageStates, nil | |||
} | |||
|
|||
func getRevisionFromCodePipeline(pipelineName string) (string, error) { |
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.
Please create the function on the codePipelineManager
(see: ListState
). Function will need to be uppercase to make it public and then would suggest a name like GetCurrentRevision
. Then also create an interface named PipelineRevisionGetter
with the single method. Include the interface in the PipelineManager
interface.
Why?
- You have access to the connection to the codepipeline service that has already been established (see
ListState
) - Consistency with other functions
common/pipeline.go
Outdated
|
||
func getRevisionFromCodePipeline(pipelineName string) (string, error) { | ||
sess := session.Must(session.NewSession()) | ||
service := codepipeline.New(sess) |
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.
Don't start a new session...reuse connection in codepipelineiface.CodePipelineAPI
. This one is already honoring the region and profile that the user requested.
Also, this will improve your ability to unit test. Check out the unit test for ListState
and notice how you are able to mock the AWS SDK to avoid having to actually talk to the CodePipeline service.
common/pipeline.go
Outdated
} | ||
for _, stageState := range response.StageStates { | ||
for _, actionState := range stageState.ActionStates { | ||
if *actionState.ActionName == "Source" { |
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.
use aws.StringValue(actionState.ActionName)
common/pipeline.go
Outdated
} | ||
} | ||
|
||
return "", errors.New("Can not locate revision from CodePipeline: " + pipelineName) |
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.
Consider fmt.Errorf("Can not locate revision from CodePipeline: %s",pipelineName)
Fixes #80