Skip to content

Conversation

@pozeus
Copy link
Contributor

@pozeus pozeus commented Apr 12, 2021

Adding support for CodeBuild to pull images from docker hub using LinuxBuildImage classmethod from_docker_registry (https://docs.aws.amazon.com/cdk/api/latest/python/aws_cdk.aws_codebuild/LinuxBuildImage.html#aws_cdk.aws_codebuild.LinuxBuildImage.from_docker_registry).
I added some documentation and an example as well, please feel free to change if you think it's not explanatory enough.

Images can be provided like:

pipelines:
  - name: example-custom-image
    default_providers:
      source:
        ...
      build:
        provider: codebuild
        properties:
          image: bitnami/mongodb
    targets:
      - ...

@sbkok
Copy link
Collaborator

sbkok commented Apr 14, 2021

Hi @pozeus,

Thanks for your contribution here.
I like adding support for this!

I would like to propose a slight modification so we can support other registries later as well.
At the moment, you check whether the image is defined for use with CodeBuild.
If not, it does a look-up for a docker container instead.

Could you change this to check if the string starts with: "docker-hub://${image-goes-here}"?
So we can add other registries in the future too? For example, adding support for the AWS ECR Public gallery, as is described here. I'm not asking to add support for the latter right away, but by prefixing the images we don't have to introduce a breaking change later on.

Let me know if it is unclear or if I can support you with these changes.

Many Thanks,
Simon

@sbkok sbkok added the enhancement New feature or request label Apr 14, 2021
Copy link
Collaborator

@sbkok sbkok left a comment

Choose a reason for hiding this comment

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

Hi @pozeus,

Could you also update the PR to include the following statement?
This is specified as part of the template of our PRs and is required for all contributions to our repository.

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

Many thanks,
Simon

@sbkok
Copy link
Collaborator

sbkok commented May 21, 2021

Dear @pozeus,

I put the "on hold" tag on the pull request, as we cannot merge this fix before you agree to the licensing terms.
Additionally, there are a few review comments that suggest changing the implementation slightly.
Could you look at both of these points, otherwise I will have to close the pull request without merging unfortunately.
Considering the usefulness of the feature, I think that would not be ideal.

If you don't have time to work on the fix, please agree with the licensing terms and state so here, so we can work on the improvements instead.

Best regards,
Simon

Copy link
Contributor Author

@pozeus pozeus left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, I will fix this coming week.

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

@pozeus
Copy link
Contributor Author

pozeus commented May 24, 2021

@sbkok Hi Simon,
Please see the latest update. If you think there is other better way's of doing this, please provide feedback.

Copy link
Collaborator

@sbkok sbkok 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 making these improvements @pozeus.

The CI build process reported the following linting issues:

src/lambda_codebase/initial_commit/bootstrap_repository/adf-build/shared/cdk/cdk_constructs/adf_codebuild.py:161:0: C0303: Trailing whitespace (trailing-whitespace)

src/lambda_codebase/initial_commit/bootstrap_repository/adf-build/shared/cdk/cdk_constructs/adf_codebuild.py:165:65: C0303: Trailing whitespace (trailing-whitespace)

src/lambda_codebase/initial_commit/bootstrap_repository/adf-build/shared/cdk/cdk_constructs/adf_codebuild.py:196:58: C0303: Trailing whitespace (trailing-whitespace)

src/lambda_codebase/initial_commit/bootstrap_repository/adf-build/shared/cdk/cdk_constructs/adf_codebuild.py:163:4: R1710: Either all return statements in a function should return an expression, or none of them should. (inconsistent-return-statements)

Could you fix those too, so we can merge the changes after?
You can run the test suite + linting by running tox locally too.

@sbkok sbkok removed the on hold label May 25, 2021
@pozeus
Copy link
Contributor Author

pozeus commented May 25, 2021

@sbkok I've now fixed the return which was missing in the function and added the suggested example. Please review 😊

@pozeus
Copy link
Contributor Author

pozeus commented Jun 8, 2021

@sbkok Any comment regarding this?

sbkok
sbkok previously approved these changes Nov 5, 2021
Copy link
Collaborator

@sbkok sbkok left a comment

Choose a reason for hiding this comment

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

Perfect. Thanks for making these changes and contributing this feature!

@sbkok sbkok requested a review from StewartW November 5, 2021 19:19
@sbkok sbkok added this to the v3.2.0 milestone Nov 5, 2021
Copy link
Contributor

@StewartW StewartW left a comment

Choose a reason for hiding this comment

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

Looks good to me. :shipit:

@sbkok sbkok merged commit 20ff7d3 into awslabs:master Nov 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants