concourse / semver-resource

automated semantic version bumping
Apache License 2.0
97 stars 105 forks source link

Adding support for assume role arn #142

Closed lnguyen closed 1 year ago

dsboulder commented 1 year ago

Wow thanks for helping out, BOSH folks!

ameowlia commented 1 year ago

👋 @lnguyen,

Thank you so much for this PR.

❓ Can you please add (1) tests and (2) docs for this change?

lnguyen commented 1 year ago

Hello @ameowlia ! I don't think there's a great way for us to test this without creating an assume role for the pipeline for this and using that. I did however changed the field to be assume_role_arn instead of role_arn and added it to readme

ameowlia commented 1 year ago

Thanks for updating the readme @lnguyen 👍

I don't think there's a great way for us to test this without creating an assume role for the pipeline for this and using that.

This surprises me. I am definitely not an expert here and I am just trying to help give quick feedback on PRs. So take all of this with a grain of salt. But I think this is a unit test for the component you are updating which does not require real AWS credentials. I think it just tests that the driver is configured properly. You are not making any new AWS calls in your PR, which leads me to believe that a real AWS account is not necessary for unit testing. I would suspect that you could write a similar unit test that AssumeRoleArn is present, then that credential is included in the driver.

😄 I started looking at this PR because I was working on a track of work that is blocked by it and I am (perhaps too) quick to give my opinions. We should definitely also have a concourse expert (@rui42?) look at it as well.

ram-pivot commented 1 year ago

I was able to use the forked repo with the AssumeRole changes and test it in a pipeline.

My steps:

  1. Ran docker build to get an image with Assume Role feature, tagged the image and pushed it to an image registry.

    docker build -t semver-resource -f dockerfiles/ubuntu/Dockerfile .
    docker tag semver-resource:latest image-registry
    docker push image-registry
  2. Created a test pipeline to use the newly created semver-resource image and used that image to do testing with assume role access keys.

resource_types:
- name: semver
  type: docker-image
  source:
    repository: YOUR-IMAGE-REGISTRY/semver-resource

resources:
- name: semver-fork
  type: semver
  source:
    driver: s3
    bucket: semver-testing
    key: version
    access_key_id: ((access_key_id))
    secret_access_key: ((secret_access_key))
    assume_role_arn: ((assume_role_arn))
    region_name: us-east-2
    initial_version: 0.0.1
jobs:
- name: some-job
  plan:
  - get: semver-fork
    params:
      bump: major
  - put: semver-fork
    params: {file: semver-fork/version} 
  1. I was able to verify that the changes work fine.