Skip to content

Conversation

reakaleek
Copy link
Member

@reakaleek reakaleek commented Apr 27, 2023

What does this pull request do?

Based on #1796

Relevant commit range: https://github.com/elastic/apm-agent-python/pull/1814/files/766da7e9b5e9f8c4d5067ffac3f4c4087ade5554..436c7f5c8748bfb7fffac092bd763e963dc2f3ac

  • Adds lambda layer release automation
  • Add creation of github release draft to facilitate the arn file

How to test

ELASTIC_LAYER_NAME=test ./.ci/publish-aws.sh

With valid AWS credentials

@github-actions github-actions bot added agent-python community Issues opened by the community triage Issues awaiting triage labels Apr 27, 2023
@reakaleek reakaleek requested review from a team, basepi and beniwohli April 27, 2023 11:44
@reakaleek reakaleek marked this pull request as ready for review April 27, 2023 11:51
Comment on lines 63 to 141
github-draft:
needs:
- publish-lambda-layers
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- uses: actions/download-artifact@v3
with:
name: arn-file
- run: >-
gh release create "${GITHUB_REF_NAME}"
--title="${GITHUB_REF_NAME}"
--generate-notes
--notes-file=".arn-file.md"
--draft
env:
GH_TOKEN: ${{ github.token }}
Copy link
Member Author

Choose a reason for hiding this comment

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

@basepi

I'm creating a github release draft here, which can be adjusted and published manually later.

I did this so we have the arn table already in the draft.

example arn table created during testing

Elastic APM Python agent layer ARNs

Region ARN
ap-northeast-1 arn:aws:lambda:ap-northeast-1:627286350134:layer:jcal-dev-elastic-apm-python:6
ap-northeast-2 arn:aws:lambda:ap-northeast-2:627286350134:layer:jcal-dev-elastic-apm-python:6
ap-northeast-3 arn:aws:lambda:ap-northeast-3:627286350134:layer:jcal-dev-elastic-apm-python:6
ap-south-1 arn:aws:lambda:ap-south-1:627286350134:layer:jcal-dev-elastic-apm-python:8
ap-southeast-1 arn:aws:lambda:ap-southeast-1:627286350134:layer:jcal-dev-elastic-apm-python:6
ap-southeast-2 arn:aws:lambda:ap-southeast-2:627286350134:layer:jcal-dev-elastic-apm-python:6
ca-central-1 arn:aws:lambda:ca-central-1:627286350134:layer:jcal-dev-elastic-apm-python:6
eu-central-1 arn:aws:lambda:eu-central-1:627286350134:layer:jcal-dev-elastic-apm-python:6
eu-north-1 arn:aws:lambda:eu-north-1:627286350134:layer:jcal-dev-elastic-apm-python:6
eu-west-1 arn:aws:lambda:eu-west-1:627286350134:layer:jcal-dev-elastic-apm-python:6
eu-west-2 arn:aws:lambda:eu-west-2:627286350134:layer:jcal-dev-elastic-apm-python:6
eu-west-3 arn:aws:lambda:eu-west-3:627286350134:layer:jcal-dev-elastic-apm-python:6
sa-east-1 arn:aws:lambda:sa-east-1:627286350134:layer:jcal-dev-elastic-apm-python:6
us-east-1 arn:aws:lambda:us-east-1:627286350134:layer:jcal-dev-elastic-apm-python:6
us-east-2 arn:aws:lambda:us-east-2:627286350134:layer:jcal-dev-elastic-apm-python:6
us-west-1 arn:aws:lambda:us-west-1:627286350134:layer:jcal-dev-elastic-apm-python:6
us-west-2 arn:aws:lambda:us-west-2:627286350134:layer:jcal-dev-elastic-apm-python:6

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks great!

@reakaleek reakaleek marked this pull request as draft April 27, 2023 12:00
@reakaleek reakaleek self-assigned this Apr 27, 2023
@reakaleek reakaleek marked this pull request as ready for review April 27, 2023 12:15
@reakaleek
Copy link
Member Author

@elastic/apm-agent-python

Not sure if related, but the windows tests seem to be stuck.

Copy link
Contributor

@amannocci amannocci left a comment

Choose a reason for hiding this comment

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

LGTM

Note: Some minor feedbacks

Copy link
Member

@v1v v1v left a comment

Choose a reason for hiding this comment

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

LGTM, with some comments

--region="${region}" \
--layer-name="${FULL_LAYER_NAME}" \
--description="AWS Lambda Extension Layer for the Elastic APM Python Agent" \
--license="Apache-2.0" \
Copy link
Member

Choose a reason for hiding this comment

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

As far as I see the apm-agent-python uses a different license. But I don't know what's the one for the AWS lambda, can we clarify this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. It's copied from apm-aws-lambda. I mistakenly assumed it will be the same.

Copy link
Member Author

Choose a reason for hiding this comment

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

Any suggestions @basepi @beniwohli ?

Copy link
Contributor

@basepi basepi Apr 27, 2023

Choose a reason for hiding this comment

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

The Layer will be licensed with BSD 3 Clause, same as the agent: https://github.com/elastic/apm-agent-python/blob/main/LICENSE

(This is due to legacy code, Apache-2.0 would definitely be my preference but we're stuck with the original license.)

Copy link
Member Author

Choose a reason for hiding this comment

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

- uses: actions/download-artifact@v3
with:
name: arn-file
- run: >-
Copy link
Member

Choose a reason for hiding this comment

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

NIT: Use name for the step?

Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
- run: >-
- name: Create GitHub Draft Release
run: >-

if: always()
needs:
- publish-lambda-layers
- release
Copy link
Member

Choose a reason for hiding this comment

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

What about github-draft? Though, it has a dependency with publish-lambda-layers, if added, will it work in the case github-draft failed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
- release
- publish-pypi
- github-draft

Copy link
Contributor

@basepi basepi left a comment

Choose a reason for hiding this comment

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

Looking great! Thank you for the fast turnaround here. Once we fix up the license and the other review feedback we should be good.

@reakaleek
Copy link
Member Author

reakaleek commented Apr 27, 2023

@basepi one last thing that I noticed while checking the license.

what are the compatible runtimes?

these are the available options:

python2.7
python3.6
python3.7
python3.8

can be one or more

@basepi
Copy link
Contributor

basepi commented Apr 27, 2023

what are the compatible runtimes?

3.6, 3.7, 3.8, 3.9 (3.9 is an option too, it's just up at the top of the dropdown with the other "latest versions")

@reakaleek
Copy link
Member Author

what are the compatible runtimes?

3.6, 3.7, 3.8, 3.9 (3.9 is an option too, it's just up at the top of the dropdown with the other "latest versions")

Ah ups, thanks for double checking. I was looking at the example of the cli documentation

https://awscli.amazonaws.com/v2/documentation/api/2.0.34/reference/lambda/publish-layer-version.html and the list is incomplete.

@reakaleek
Copy link
Member Author

3.6, 3.7, 3.8, 3.9 (3.9 is an option too, it's just up at the top of the dropdown with the other "latest versions")

18f561b

Copy link
Member

@v1v v1v left a comment

Choose a reason for hiding this comment

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

LGTM, with a minor comment

tags:
- "v*.*.*"

permissions:
Copy link
Member

Choose a reason for hiding this comment

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

NIT: if it is only used when creating the draft release, then it can be configured in that particular job rather than globally

Copy link
Member Author

Choose a reason for hiding this comment

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

@basepi
Copy link
Contributor

basepi commented Apr 28, 2023

Windows tests are unrelated: #1817

@reakaleek
Copy link
Member Author

reakaleek commented May 2, 2023

@basepi

To be sure: are we good to merge this PR including the changes from #1796?

@basepi
Copy link
Contributor

basepi commented May 2, 2023

@reakaleek are we good to merge this PR including the changes from #1796?

Please do not merge this. #1796 needs to be reviewed and merged first, then we'll merge the automation.

@basepi
Copy link
Contributor

basepi commented May 5, 2023

@reakaleek I updated #1796 to better handle the docker image paths, and added a Dockerfile. You'll want to rebase.

Similar to the nodejs docker file, it expects an AGENT_DIR arg, which in the this case should be ./build/dist/package/python.

This will result in the same directory structure in both the lambda layer (mounted at /opt) and the docker image.

Can you please add support for building and uploading the docker image? Thanks!

@reakaleek reakaleek force-pushed the feature/lambda-layer-release-automation branch 3 times, most recently from 33a23a1 to 489cffe Compare May 8, 2023 10:35
@reakaleek
Copy link
Member Author

@reakaleek I updated #1796 to better handle the docker image paths, and added a Dockerfile. You'll want to rebase.

Similar to the nodejs docker file, it expects an AGENT_DIR arg, which in the this case should be ./build/dist/package/python.

This will result in the same directory structure in both the lambda layer (mounted at /opt) and the docker image.

Can you please add support for building and uploading the docker image? Thanks!

63b7294

@reakaleek reakaleek requested a review from basepi May 8, 2023 20:36
@reakaleek reakaleek requested a review from a team May 9, 2023 10:14
@basepi
Copy link
Contributor

basepi commented May 9, 2023

#1796 is merged, so you can rebase and merge this at your convenience.

@basepi
Copy link
Contributor

basepi commented May 9, 2023

Actually, I have one more request, @reakaleek -- I think we should be testing make-distribution.sh to make sure we don't end up with a dependency conflict in dev-utils/requirements.txt. Since I'm dynamically injecting into requirements.txt at build time, Dependabot can't properly check for incompatibilities in the dependency chain. I think we should be testing this regularly, rather than finding out it fails when we try to release.

I could throw a set -e at the beginning of the script and then write a unit test that runs this command. But I think it probably makes sense to run it as a github action. Do you agree?

Edit: We already do set -o errexit which is equivalent to set -e so we just need to run it and check the retcode.

@reakaleek
Copy link
Member Author

Actually, I have one more request, @reakaleek -- I think we should be testing make-distribution.sh to make sure we don't end up with a dependency conflict in dev-utils/requirements.txt. Since I'm dynamically injecting into requirements.txt at build time, Dependabot can't properly check for incompatibilities in the dependency chain. I think we should be testing this regularly, rather than finding out it fails when we try to release.

I could throw a set -e at the beginning of the script and then write a unit test that runs this command. But I think it probably makes sense to run it as a github action. Do you agree?

Edit: We already do set -o errexit which is equivalent to set -e so we just need to run it and check the retcode.

Hi @basepi,

Sorry for the delay, just got back from sick leave.

I think we should be testing this regularly, rather than finding out it fails when we try to release.

I agree, testing this regularly makes sense.

I could throw a set -e at the beginning of the script and then write a unit test that runs this command. But I think it probably makes sense to run it as a github action. Do you agree?

Can you elaborate on this more? Do you mean we run the script make-distribution.sh on a PR level in it's own GH workflow job? Or do you mean we should create a custom action that does it?

@reakaleek reakaleek force-pushed the feature/lambda-layer-release-automation branch from 63b7294 to beeab0c Compare May 15, 2023 09:28
@reakaleek
Copy link
Member Author

Actually, I have one more request, @reakaleek -- I think we should be testing make-distribution.sh to make sure we don't end up with a dependency conflict in dev-utils/requirements.txt. Since I'm dynamically injecting into requirements.txt at build time, Dependabot can't properly check for incompatibilities in the dependency chain. I think we should be testing this regularly, rather than finding out it fails when we try to release.
I could throw a set -e at the beginning of the script and then write a unit test that runs this command. But I think it probably makes sense to run it as a github action. Do you agree?
Edit: We already do set -o errexit which is equivalent to set -e so we just need to run it and check the retcode.

Hi @basepi,

Sorry for the delay, just got back from sick leave.

I think we should be testing this regularly, rather than finding out it fails when we try to release.

I agree, testing this regularly makes sense.

I could throw a set -e at the beginning of the script and then write a unit test that runs this command. But I think it probably makes sense to run it as a github action. Do you agree?

Can you elaborate on this more? Do you mean we run the script make-distribution.sh on a PR level in it's own GH workflow job? Or do you mean we should create a custom action that does it?

I extracted the build-distribution script into a reusable workflow and it's now called in the release and the test workflow.

Let me know if this matches your suggestion/request.

2f2253e

@reakaleek reakaleek requested a review from basepi May 15, 2023 11:54
@basepi
Copy link
Contributor

basepi commented May 16, 2023

I extracted the build-distribution script into a reusable workflow and it's now called in the release and the test workflow.

Let me know if this matches your suggestion/request.

This should be perfect! Thank you.

@reakaleek reakaleek merged commit 2898e3f into elastic:main May 17, 2023
@reakaleek reakaleek deleted the feature/lambda-layer-release-automation branch May 17, 2023 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent-python community Issues opened by the community triage Issues awaiting triage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants