ci/prevent init file to break packaging #11914
Conversation
S3 Image Test Results (AMD64 / ARM64) 2 files 2 suites 3m 52s ⏱️ Results for commit 7e3c877. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Thanks a lot for making sure that we learn from every single issue!
I am not 100% sure though if this really is necessary. There are an infinite amount of ways to break the downstream project with changes in community, but there are lots of different pipelines which should ensure that this doesn't happen.
Here in the Community project, we have the "Community Integration Tests against Pro" which should inform us about changes in PRs which will break the downstream project.
Do you think this specific check is necessary given that we have all these checks already in place?
Makefile
Outdated
| @@ -117,6 +117,7 @@ test-coverage: TEST_EXEC = python -m coverage run $(COVERAGE_ARGS) -m | |||
| test-coverage: test ## Run automated tests and create coverage report | |||
|
|
|||
| lint: ## Run code linter to check code style, check if formatter would make changes and check if dependency pins need to be updated | |||
| @[ -f localstack-core/localstack/__init__.py ] && echo "localstack-core/localstack/__init__.py will break packaging. Please delete end retry" && exit 1 || : | |||
There was a problem hiding this comment.
question: The presence of the init dunder kills the build and all tests of the downstream project, right? Or is it just the linting?
There was a problem hiding this comment.
You are correct, it kills the build. It will break any import to localstack.XXX when discovering the plugins.
None of these checks succeeded to capture the issue until the attempting to build the downstream project. The issue was somewhat difficult to narrow down as there can be multiple commits to the Community repo since last build and a single init file added can easily be missed or overlooked. I don't think the check is a necessity, as there is no risk to publish a broken image (as it won't build). But I do think it is a low cost, low impact check that can prevent losing more time in the future to debugging an known issue. I appreciate it is not realistic to make extra checks to prevent every single problem, and we need to be careful about what we commit and approve. I thought that given the time invested in tracking it down it would be beneficial to prevent the same error in the future. Though, as a known issue, we should be able to get to it more swiftly. |
|
I'm also leaning on the side of this being a reasonable low-cost check potentially avoiding hours of unnecessary debugging. Simply checking that this contract keeps being honored seems to be a sensible pre-flight check, particularly as something that is extremely easy to accidentally introduce and also extremely hard to debug without specific context information. |
There was a problem hiding this comment.
Thanks for answering my questions on the PR! Sound reasoning, a simple check preventing a hard-to-find easy-to-introduce regression :)
Just one more question which we can follow up in another PR / this shouldn't block this PR:
None of these checks succeeded to capture the issue until the attempting to build the downstream project. The issue was somewhat difficult to narrow down as there can be multiple commits to the Community repo since last build and a single init file added can easily be missed or overlooked.
That is very surprising to me. How would the Community Integration Tests against Pro work with the init dunder present? Is it because the runs were just cancelled to get it over the line as fast as possible and the pipeline would have in fact shown the issue? Or is there something we should also fix in that pipeline?
|
Thanks for the feedback @alexrashed, always appreciated. The pipeline did execute successfully as can be seen here. This is from #11909 which was merged after #11906 that added the init file. I think it might be because localstack is installed in editable mode. I tested locally and was able to reproduce. With That seems to be a good next point of investigation 😉. It seems safe to do a regular install instead, but would overall be a more costly fix. I am happy to keep the conversation going and potentially update in a follow up. |
Motivation
During 4.0.1 deploy the -ext build failed because of a namespace issue. Some tricky debugging lead to finding the issue stemmed from the addition of
localstack-core/localstack/__init__.py. The result was thatlocalstack.pro.corecould no longer be found.Pr fixing: #11012 and #11913
Kudos @bentsku for finding the issue
Changes
Add a check during linting that will raise loud and clear the error if and when this would happen again.
From ci run https://app.circleci.com/pipelines/github/localstack/localstack/29774/workflows/8c2feac2-9780-471e-9e96-ab1005811f4f