Skip to content
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

Adds support to delete old versions when successful deployment #7414

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

driverpt
Copy link

@driverpt driverpt commented Aug 27, 2024

Which issue(s) does this change fix?

#7398

Why is this change necessary?

Because there's a limit on how many versions a Lambda has.

How does it address the issue?

Automatically deletes old version. Very useful when on SnapStart

What side effects does this change have?

None, it's disabled by default

Mandatory Checklist

PRs will only be reviewed after checklist is complete

  • Add input/output type hints to new functions/methods
  • Write design document if needed (Do I need to write a design document?)
  • Write/update unit tests
  • Write/update integration tests
  • Write/update functional tests if needed
  • make pr passes
  • make update-reproducible-reqs if dependencies were changed
  • Write documentation

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@driverpt driverpt requested a review from a team as a code owner August 27, 2024 09:17
@github-actions github-actions bot added pr/external stage/needs-triage Automatically applied to new issues and PRs, indicating they haven't been looked at. labels Aug 27, 2024
@driverpt driverpt force-pushed the add-auto-delete-old-version branch 2 times, most recently from f414471 to 26d1e47 Compare September 2, 2024 07:43
@jysheng123
Copy link
Contributor

Hi, Thanks for bringing up this PR. The code looks good to me so Ill run the approval workflow to get the testing process started, but can you also create an integration tests testing the happy path from beginning to end?

@jysheng123
Copy link
Contributor

Can you also run make black to fix the formatting changes?

Copy link
Contributor

@lucashuy lucashuy 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 opening this PR, left some comments. Additionally, we might want to consider adding this to the sam deploy command. This might need some more thought though if this makes sense to you and the team.

samcli/lib/sync/flows/function_sync_flow.py Outdated Show resolved Hide resolved
version = self._lambda_client.publish_version(FunctionName=function_physical_id).get("Version")
self._local_sha = str_checksum(str(version), hashlib.sha256())
LOG.debug("%sCreated new function version: %s", self.log_prefix, version)
if version:
self._lambda_client.update_alias(
FunctionName=function_physical_id, Name=self._alias_name, FunctionVersion=version
)
if self._delete_old_alias and current_alias_version:
function_name_w_version = "{}:{}".format(function_physical_id, current_alias_version)
self._lambda_client.delete_function(FunctionName=function_name_w_version)
Copy link
Contributor

Choose a reason for hiding this comment

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

Will it ever be possible for this to be my_function:$LATEST? Just wanted to ask what happens if the Lambda alias resolved to the latest version of the function, and if deleting my_function:$LATEST just deletes the function.

Copy link
Author

@driverpt driverpt Nov 1, 2024

Choose a reason for hiding this comment

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

No, because there is no current alias. It only deletes old alias if there is an existing alias

current_alias_version is the lambda.get_alias operation. Does $LATEST qualify as an alias?

samcli/lib/sync/flows/alias_version_sync_flow.py Outdated Show resolved Hide resolved
@lucashuy
Copy link
Contributor

Hi @driverpt, just wanted to check in about the state of this PR and if you had the capacity to work on it or if there was anything you needed to move forward

@driverpt
Copy link
Author

Hello. I can work on it. But I'm waiting on the CF Spec to be discussed

@lucashuy
Copy link
Contributor

I've chatted with some team members, and the Metadata section is likely a better location, than inside of Properties. This won't require a spec change, and will only impact SAM CLI as the Metadata doesn't impact SAM transform. Having a CLI option was also brought up, but using an option here might make the UX a little messy in terms of deciding how we want to pass in function names into the option.

@lucashuy lucashuy added area/sync sam sync command and removed stage/needs-triage Automatically applied to new issues and PRs, indicating they haven't been looked at. labels Sep 25, 2024
@mndeveci
Copy link
Contributor

Hey @driverpt looking at the discussion here, I saw that recommendation is to use Metadata of the resources. Please let us know when you can follow-up with those changes, so that we can continue the review.

Thanks!

@hnnasit
Copy link
Contributor

hnnasit commented Oct 30, 2024

Hi @driverpt, following up to check if you were planning to make the above requested changes and had any questions the team could help with.

@driverpt
Copy link
Author

Hello. Haven't yet had the time to implement this. I'll probably take a look at it this weekend

Copy link
Contributor

@mndeveci mndeveci 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 your contribution, left a comment about one method response where you wrapped it up with str method.

Comment on lines +121 to +125
return str(
self._lambda_client.get_alias(
FunctionName=self.get_physical_id(self._function_identifier), Name=self._alias_name
).get("FunctionVersion")
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrapping .get method response with str will cause None value to be returned as "None" (string), so the IF statement above will not work as intended.

Suggested change
return str(
self._lambda_client.get_alias(
FunctionName=self.get_physical_id(self._function_identifier), Name=self._alias_name
).get("FunctionVersion")
)
return self._lambda_client.get_alias(
FunctionName=self.get_physical_id(self._function_identifier), Name=self._alias_name
).get("FunctionVersion")

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @driverpt is there any chance you can address this comment? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/sync sam sync command pr/external
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants