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

Added throw error when event function name is long #1205

Merged
merged 16 commits into from Oct 19, 2023

Conversation

goya813
Copy link
Collaborator

@goya813 goya813 commented Dec 3, 2022

Description

  • Added exception handling for cases where event function names are long.
  • The problem is that if the function name is too long, the function name is cut off and lambda cannot be called from the Event Bridge.

GitHub Issues

@goya813 goya813 requested a review from monkut December 3, 2022 15:57
@coveralls
Copy link

coveralls commented Dec 3, 2022

Coverage Status

coverage: 74.736% (-0.03%) from 74.769% when pulling 611e79d on feature/issue-1036-validation-event-settings into 8d68b54 on master.

zappa/core.py Outdated Show resolved Hide resolved
zappa/core.py Show resolved Hide resolved
zappa/core.py Outdated Show resolved Hide resolved
zappa/core.py Outdated Show resolved Hide resolved
zappa/core.py Outdated Show resolved Hide resolved
zappa/core.py Show resolved Hide resolved
zappa/core.py Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@monkut monkut merged commit 7a9a5ad into master Oct 19, 2023
6 of 12 checks passed
@javulticat
Copy link
Member

Hey there folks - I was looking over this PR this evening and thinking about the issue it aims to solve and I got to wondering... is this PR actually necessary? What problem is it actually solving? And for whom?

I know AWS has a character limit for event function names, but should our solution to that be to throw another Exception at our users for each of their event function names that exceed that limit? Because I believe we resolved the handling of that several years ago in a more elegant way with #1080, and I would've expected that #1080 would've been able to solve the issue referenced here (#1036) as well, since it allowed for event function names of any length to be given to Zappa's config, and it would automatically handle making sure that they ended up with nice, uniquely identifiable names that conformed to AWS limits.

Now, since #1080 was actually merged several months after #1036 was opened, the user who had the complaint in that issue may not have gotten a chance to try the new functionality out that may have solved the issue they were having. In any case, the user has never once returned to follow up on the issue since they made it over 2 years ago (and if we were on top of our issue pruning, that issue would have been closed as stale over a year ago 😄), so it doesn't seem like it caused them too significant of a problem - perhaps they tried a release with #1080 present in it shortly afterward and that solved their problems after all.

In any case, I'm especially curious then why we felt the need to prioritize handling a 2+ year old stale issue that, in all likelihood, sounds like it may have already been fixed years ago by #1080. And, that aside, I'm also a little confused by the approach found in this PR (#1205). If I understand correctly, this PR seems like it would change the behavior introduced in #1080 so that now, a user including a long event function name in a Zappa config will halt the deploy/update process in its tracks, and put the onus on the user to manually rename any/all of their event function names which are too long by AWS standards. And better still, it looks like the Exception it gives is for the first event function name it finds that is too long, even if there are hundreds. So, are users just expected to try to run zappa deploy hundreds of times and rename the event function names called out by the Exception one-by-one until they no longer hit this new Exception?

I've worked at places where we had literally thousands of event functions with names in our Zappa config that exceeded the AWS character limit, which was one of my motivators for developing #1080 - it was literally faster to create that PR that to rename all of our event functions and find and rename them in all of the places they may have been named in other repos/documentation/wikis/trainings/etc. within my organization. If we got an error telling us we needed to change a different event function name each time we tried to deploy 5000+ times in a row, that wouldn't exactly be a great user experience (and a great waste of a week).

So, anyway, to conclude my old man shaking fist at sky rambling, my first impression of this PR is that it might be a regression that puts work we've solved years ago for our users right back onto our users' plates. So, what is the compelling reason for why we need to now halt users' deploy processes to explicitly tell them that they need to manually shorten their event function names one-by-one rather than have Zappa continue to shorten all of their event function names for them automatically like it has been doing without incident for years?

Maybe I'm missing something obvious (wouldn't be the first time 😄), but I'm just not sure I understand the thought process that led to this PR being considered necessary to create, for numerous reasons.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants