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 a flag 'skip_package_lock_generation' to FTL #741

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Adds a flag 'skip_package_lock_generation' to FTL #741

wants to merge 3 commits into from

Conversation

belchatek
Copy link
Contributor

Adds a flag 'skip_package_lock_generation' to control if FTL should generate package-lock file at the beginning of the run. Skiping this can improve build speed but can cause older version of dependencies to be used.

…enerate package-lock file at the beginning of the run. Skiping this can improve build speed but can cause older version of depe

ndencies to be used.
@belchatek
Copy link
Contributor Author

@aaron-prindle
Please review

Copy link
Collaborator

@bobcatfish bobcatfish left a comment

Choose a reason for hiding this comment

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

Can you explain a bit more about why you want to skip generating the lock file and reuse the cache key?

@@ -68,6 +68,7 @@ py_test(
srcs = ["common/util_test.py"],
deps = [
":ftl_lib",
"@mock",
Copy link
Collaborator

Choose a reason for hiding this comment

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

was it a bug that this was missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was unable to run this test without . this dependency.

@belchatek
Copy link
Contributor Author

Generating package-lock file is quite time consuming process - it can take even 5-10 seconds. I want to have an option to avoid paying this cost to check if there is cached layer available.

@aaron-prindle
Copy link
Collaborator

Nice, this change definitely makes sense.

it seems that this is failing a lint check that we have:
./ftl/node/layer_builder.py:54:80: E501 line too long (89 > 79 characters)
If you can fix that error and re submit the PR tests should run.

Also can you add an integration test for the flag that you are adding?
You will need to add the test to:
https://github.com/GoogleCloudPlatform/runtimes-common/blob/master/ftl/integration_tests/ftl_node_integration_tests_yaml.py#L14

The flag to the test similar to as done here:
https://github.com/GoogleCloudPlatform/runtimes-common/blob/master/ftl/integration_tests/ftl_node_integration_tests_yaml.py#L43

And add a directory for the app to be tested, similar to here:
https://github.com/GoogleCloudPlatform/runtimes-common/tree/master/ftl/node/testdata/packages_test

And you should create a structure_test.yaml that verifies that a package.lock file is not created:
https://github.com/GoogleCloudPlatform/runtimes-common/blob/master/ftl/node/testdata/packages_test/structure_test.yaml

@belchatek
Copy link
Contributor Author

There is a problem with adding an integration test for this flag. Even if I set it package-lock.json will be created in build phase.

@belchatek
Copy link
Contributor Author

I dont understand what is broken in kokoro run

@aaron-prindle
Copy link
Collaborator

The kokoro issue here is related to our kokoro pipeline recently upgrading the version of bazel that it uses and the files in runtimes-common not being compatible with this new version. We are working on a fix currently, I will update this PR as kokoro is fixed.

@aaron-prindle
Copy link
Collaborator

@belchatek friendly ping. The CI issues you were experiencing are resolved now, resubmit and kokoro should have sane logs for the FTL tests

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