-
Notifications
You must be signed in to change notification settings - Fork 132
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
Fix update loop for Function.lambda resource #1266
Conversation
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.
Thank you @turkenf, left some comments for discussion. If the discussions are correct, then I think this works more or less as intended and to improve the situation, we can disable the late-initialization for the hash field (so that users don't have to update the hash if they don't care about it) because late-initialization forces them to care about the field :)
@turkenf, thanks for documenting the manual testing steps. We should also consider capturing this know-how in the uptest manifests (probably by executing them via the pre hooks) and automating the testing. We may even consider using an already available test image (I don't know its cost implications tbh). |
I'm very supportive of @ulucinar's suggestion to fix this by disabling late initialization. It seems like that should fix the issues, but I'd also like to try some creative ideas to break it that will hopefully fail, both for zip lambdas and image-based ones. AWS Lambda requires that its images come from AWS ECR in the same region as the lambda, but they can be from any aws account, as long as permissions are set appropriately. So I think it would work well to have a small, static lambda image hosted in ECR in a different aws account than is used for testing (so it doesn't get cloud-nuke'd), that we could use for testing from any aws account. It's worth checking, but I'd expect the cost to be minimal. |
/test-examples="examples/lambda/v1beta1/function.yaml" |
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.
Hi @turkenf,
It could be great if we could also extend the Go field documentation (which also becomes the CRD field documentation) to explain the requirement on the sourceCodeHash
(that it should be the actual computed hash if specified).
Signed-off-by: Fatih Türken <turkenf@gmail.com>
Signed-off-by: Fatih Türken <turkenf@gmail.com>
27715c2
to
16648a7
Compare
Signed-off-by: Fatih Türken <turkenf@gmail.com>
I close and re-open to trigger jobs. |
/test-examples="examples/lambda/v1beta1/function.yaml" Uptest run: https://github.com/crossplane-contrib/provider-upjet-aws/actions/runs/8818842854 |
Description of your changes
This PR fixes the update loop in the
Function.lambda
resource.Fixes: #1027
I have:
make reviewable
to ensure this PR is ready for review.backport release-x.y
labels to auto-backport this PR if necessary.How has this code been tested
Repository.ecr
resource by applying the following:aws ecr get-login-password --region us-east-1 | docker login --username AWS --password-stdin <account_id>.dkr.ecr.us-east-1.amazonaws.com
docker build -t test-repo .
docker tag test-repo:latest <account_id>.dkr.ecr.us-east-1.amazonaws.com/test-repo:latest
docker push <account_id>.dkr.ecr.us-east-1.amazonaws.com/test-repo:latest
Function.lambda
resource by applying the following:imageUri
in theFunction.lambda
resource with the new one.