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

Add linux_pre_run_command to soundness #66

Merged
merged 2 commits into from
Nov 27, 2024

Conversation

rnro
Copy link
Contributor

@rnro rnro commented Nov 19, 2024

Add a linux_pre_run_command input parameter to the soundness workflow jobs to allow callers to set up any dependencies or perform any other preparations before the soundness checks are executed.

This is useful for example for repositories which require dependencies to be installed before the build.

The if: statement evaluates to false if the input is not defined or is set to "" so the step does not appear in output.

Add a `linux_pre_run_command` input parameter to the soundness workflow
jobs to allow callers to set up any dependencies or perform any other
preparations before the soundness checks are executed.

This is useful for example for repositories which require dependencies
to be installed before the build.
@rnro rnro requested a review from a team as a code owner November 19, 2024 10:52
@@ -63,6 +63,10 @@ on:
type: boolean
description: "Boolean to enable the Python lint check job. Defaults to true."
default: true
linux_pre_run_command:
Copy link
Member

Choose a reason for hiding this comment

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

Should this really be executed before every soundness check. It seems like this is only relevant for the ones that need to build something e.g. docs and api break

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 wondered that myself but I think it's plausible that a caller may want to do some other source preparations, munging, generation that this would permit.

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'm not set on it being this way though if you'd rather we solve the immediate requirement.

Copy link
Contributor

@ktoso ktoso Nov 19, 2024

Choose a reason for hiding this comment

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

swift-java needs a bunch of weird stuff, like installing a JDK for builds to work.

I agree that it is only for things which "build" the formatting check for example does no need it. At least so far AFAICS, let's start with that and we can extend if necessary?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe have a more specific name, like linux_pre_build_command, and only run it in actions that build? And in the future, there can be another linux_pre_foo_command that doesn't interfere with the build one, and could be added to all of them (e.g. to print the environment or something).

Copy link
Member

Choose a reason for hiding this comment

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

On a side note. I am wondering if the longer term strategy is to allow overriding the docker image.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I'm happy taking this approach. Thanks for the feedback everyone.

@FranzBusch FranzBusch merged commit aced105 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.

4 participants