DRAFT control-service: AWS Code commit integration #3304
Conversation
|
@SurajViitk, you must sign our contributor license agreement before your changes are merged. Click here to sign the agreement. If you are a VMware employee, read this for further instruction. |
SurajViitk
changed the title
|
@SurajViitk, we have received your signed contributor license agreement. The review is usually completed within a week, but may take longer under certain circumstances. Another comment will be added to the pull request to notify you when the merge can proceed. |
for more information, see https://pre-commit.ci
SurajViitk
changed the title
| @@ -38,6 +38,12 @@ public class JobImageBuilder { | |||
| @Value("${datajobs.git.url}") | |||
| private String gitRepo; | |||
|
|
|||
| @Value("${datajobs.git.cc.grc}") | |||
There was a problem hiding this comment.
why can't you just re use thw git url above ?
then you don't need the if stateent below ?
There was a problem hiding this comment.
This is URL expected from Git Remote Code-commit tool, it is following format "codecommit::us-east-1://vdkdata-jobs" and only for this url format, git can fetch from AWS Code Commit repositories
There was a problem hiding this comment.
yes but can't you just set this through {datajobs.git.url} property?
There was a problem hiding this comment.
I tested that but code push through jgit didnt work in that case, so I included both the git and grc url, this is a optional property required only if datajobs.git.assumeIAMRole is true, maybe I can add a comment before this field in properties file to clarify this further
| @@ -67,7 +75,6 @@ public JobUpload( | |||
| * @return resource containing data job content in a zip format. | |||
| */ | |||
| public Optional<Resource> getDataJob(String jobName) { | |||
| CredentialsProvider credentialsProvider = gitCredentialsProvider.getProvider(); | |||
There was a problem hiding this comment.
you are changing the flow?
Are you sure provider doesn't need to be called before every operation?
There was a problem hiding this comment.
The getProvider function generates a simple CredentialProvider based on username and password which we are supplying from helm/properties, so I concluded that it's OK to call it once in constructor, rather than every time -
Same for the AWS code commit credential provider, we just need roleARN at the starting which we supplied using datajobs.aws.roleArn property
| this.gitWrapper = gitWrapper; | ||
| this.featureFlags = featureFlags; | ||
| this.authorizationProvider = authorizationProvider; | ||
| this.jobUploadAllowListValidator = jobUploadAllowListValidator; | ||
| this.jobUploadFilterListValidator = jobUploadFilterListValidator; | ||
|
|
||
| if(assumeCodeCommitIAMRole){ |
There was a problem hiding this comment.
I don't really like this.
I would prefer if you could create a single provider bean outside of the class and pass it in using spring.
Please see this PR as an exampel https://github.com/vmware/versatile-data-kit/pull/3269/files
There was a problem hiding this comment.
Updated the code according to your reference
for more information, see https://pre-commit.ci
|
Looks good to me. @mivanov1988 is one who have worked al ot on job-builder so if he can take a look that would be a bonus. How did you test it? You can reuse/modify the script to deploy the control service in your own kubernetes and make sure that it works - https://github.com/vmware/versatile-data-kit/blob/main/projects/control-service/cicd/deploy-testing-pipelines-service.sh |
| datajobs.git.url=${GIT_URL} | ||
| datajobs.git.cc.grc=${GIT_GRC_URL} |
There was a problem hiding this comment.
We can just document that git.url supports both code commit and normal git URL.
Seems unnecessary to have both.
| datajobs.git.cc.grc=${GIT_GRC_URL} | ||
|
|
||
| # datajobs.git.assumeIAMRole tells the control-service if the Service Account pattern should be used for AWS CodeCommit. | ||
| datajobs.git.assumeIAMRole=${DATAJOBS_CC_AWS_ASSUME_IAM_ROLE:false} |
There was a problem hiding this comment.
New variables would also need to be exposed and documented n https://github.com/vmware/versatile-data-kit/blob/main/projects/control-service/projects/helm_charts/pipelines-control-service/values.yaml
At least there's where we've tried to sort of maintain documentation and list of control service configuration.
|
@SurajViitk, VMware has approved your signed contributor license agreement. |
Just exposing AWS credentials retreived by git to test them for AWS codecommit
EDIT: removed this