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

chore: remove serverless and terraform backends #89

Merged
merged 17 commits into from
May 9, 2023

Conversation

cyclimse
Copy link
Contributor

@cyclimse cyclimse commented May 4, 2023

Summary

What's changed?

This PR removes the serverless and terraform backends as those functionalities were not being used. Because this removes a lot of code, some structures and abstractions were simplified as well.

Why do we need this?

This improves the overall quality of the codebase and application by removing mostly dead code that was not being tested enough. In addition, we will be able to maintain this project more easily.

How have you tested it?

Adapted the integration tests. They rely on Click test CliRunner so that everything runs in a single process which makes it a lot easier to debug.

Checklist

  • I have reviewed this myself
  • There is a unit test covering every change in this PR
  • I have updated the relevant documentation

Details

  • Added a --verbose and --log-level flag to forward the SDK logs to the user (so they can get API response to debug if something goes wrong)
  • Changed the logger to a vanilla Python logger
  • Added a runtime flag to give more control of the selected runtime. Otherwise, it was impossible to deploy with newer runtimes on older versions of Python. Also, it makes the tests more reproducible.

Most of the other changes are there to clean up what's left behind:

  • Stop importing the SDK in the config package to avoid needless dependencies when deploying. This was previously impossible as we also needed PyYAML for the serverless framework.
  • Remove abstractions that were used to support multiple backends (Deployment backends, "generators" etc..).

Probably the best change out of them all is to use the click. Runner for the integration tests. This makes the integration tests run in a single process and as such debuggable. Furthermore, it removes a lot of logic in the integration tests.

description: Optional[str] = None
http_option: Optional[sdk.FunctionHttpOption] = None
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those types were changed to avoid needing to import the SDK in the config package. The config package is used by the decorators, and will be imported passively when running the functions on Scaleway. With those changes, it will be possible to vendor scw-serverless with none of its dependencies when deploying to scaleway Functions.

This is good because the next step is to add the dev command and therefore add Flask (transitively) as a dependency.

Comment on lines +14 to +15
class FunctionAPIWrapper:
"""Wraps the Scaleway Python SDK with the Framework types."""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This class was introduced to avoid the bloat in DeploymentManager

@cyclimse cyclimse force-pushed the chore/delete-multiple-backends branch from f76e6fc to 953bbf0 Compare May 9, 2023 08:30
@cyclimse
Copy link
Contributor Author

cyclimse commented May 9, 2023

Leaving a comment to remember:

When deploying to python310 with a local installation of python3.11, typing_extensions would not be installed as the python version check would be resolved by python3.11. This would cause the function to crash immediately as we imported typing_extensions in the decorators. I thought this could have been solved using the pip flag --python-version and pointing it to the runtime version. But unfortunately, this flag is only used to select wheels and does not do anything regarding the version checker. There is a pip issue tracking this: pypa/pip#11664

Realistically it's not too crucial because few packages use version markers for dependencies markers. The fix here ended up to guard the import of typing_extensions with a if TYPE_CHECKING clause. However, pip using the local python version to resolve conditionals import might be an issue for clients using different python_version / runtime combinations.

@cyclimse cyclimse requested review from Shillaker and redanrd May 9, 2023 09:52
Copy link
Collaborator

@Shillaker Shillaker left a comment

Choose a reason for hiding this comment

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

Looks great, nice work. Just one small tip on variable naming.


TEMP_DIR = "./.scw"
DEPLOYMENT_ZIP = f"{TEMP_DIR}/deployment.zip"
UPLOAD_TIMEOUT = 600 # In seconds
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would always put the unit in the variable name to avoid confusion, in this case, UPLOAD_TIMEOUT_SECONDS

@cyclimse cyclimse merged commit 71480c2 into main May 9, 2023
@cyclimse cyclimse deleted the chore/delete-multiple-backends branch May 9, 2023 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants