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

Consistently only conditionally install tools in soundness checks #68

Merged

Conversation

simonjbeaumont
Copy link
Contributor

Motivation

Some of the soundness checks rely on other tools, e.g. curl, yq, yamllint. These are installed as part of the command in the workflow. Some of them conditionally do this only if the tool wasn't detected as already installed; others check first, using which. The latter approach is better because it saves time performing the apt update and apt install commands if they are not necessary. In CI this is speed up in returning the check status and saves wasted compute. But it also has an important impact locally when running these checks is part of the developer workflow, usually with act.

Concretely, running soundness-format (an alias for the expanded act command to only run the format check) takes 38s in CI on a recent PR, but only 7s of this is actually running the check. The rest is pulling the Docker base image and doing the installation of curl.

Modifications

This PR updates all the soundness checks to be consistent with what we were already doing for the yamllint check: to only install missing tools.

As an aside, the use of apt was also inconsistent across the various soundness checks. This PR makes them consistent and:

  • Replaces use of qq with q — the use of qq is strongly discouraged in the man page without a no-action specifier.
  • Replaces use of apt-cache and apt-get with just apt, which has been supported for a long time.

Result

Faster CI, faster local workflow.

@simonjbeaumont simonjbeaumont requested a review from a team as a code owner November 26, 2024 17:31
Copy link
Member

@FranzBusch FranzBusch left a comment

Choose a reason for hiding this comment

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

LGTM!

@FranzBusch FranzBusch merged commit 04405cc into swiftlang:main Nov 27, 2024
9 checks passed
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.

2 participants