search

ome / omero-test-infra

Test infrastructure for decoupled repositories
BSD 2-Clause "Simplified" License
3 stars 14 forks source link

Run pre-commit if config file found #57

Closed manics closed 3 years ago

manics commented 3 years ago

pre-commit requires the git binary to be installed, so this will need something like https://github.com/ome/omero-web-docker/pull/54/

joshmoore commented 3 years ago

Do you have a temporary commit already making use of this? Generally makes good sense though. :+1:

manics commented 3 years ago

Not yet, it needs git, added in https://github.com/ome/omero-web-docker/pull/54

sbesson commented 3 years ago

Looks like this is becoming a requirement to move forward with https://github.com/ome/omero-cli-duplicate/pull/18, https://github.com/ome/omero-rois/pull/8 and generally any plugin where pre-commit/black is getting set-up.

What are the necessary steps to unlock the minimal amount of work required in ome/omero-web-docker#54 and be able to test this?

sbesson commented 3 years ago

Coming back to this we have now a few active plugin repositories where the new assumptions introduced with the new black/pre-commit policy introduced in omero-web are conflicting with the expectations from omero-test-infra:

A naive addition of a .flake8 file capping the line length did not solve the last repo as other warnings are conflicting and we need to maintain exclusion lists in two places.

As far as I can tell the blocker for this work was originally opened as https://github.com/ome/omero-web-docker/pull/54 where another discussion was started re deployments and Docker images. While I definitely agree with the work towards a better separation of minimal/lightweight/standalone/development images, it feels like we are not in a place to make immediately the set of decisions required to release these images.

From my side there are really two options to move forward: either we add the infrastructure to support OMERO Python library/plugins using pre-commit .pre-commit-config.yaml or we decide we cannot support it for the time being and revert to the old style flake8-only compliance checks.

In the first case, what is the minimal set of changes that need to happen to make it viable? Could the Docker image be modified in this repository for now and then propagate the change upstream? Also can we fallback to the old behavior as we are rolling out pre-commit to all repositories or will we need to update all of them to work with the new omero-test-infra?

In the second case, does it mean reverting the pre-commit changes and workflows?

manics commented 3 years ago

https://github.com/ome/omero-rois/pull/8 is passing with the latest commits

snoopycrimecop commented 3 years ago

Conflicting PR. Removed from build OMERO-plugins-push#588. See the console output for more details. Possible conflicts:

--conflicts Conflict resolved in build OMERO-plugins-push#589. See the console output for more details.

sbesson commented 3 years ago

Bringin here a related discussion that happens today due to the introduction of pre-commit GitHub workflows in few repositories - see https://github.com/ome/ome-zarr-py/commit/4ac403c4b52de88d1d0653d4b16552756eca6ffe and more recently https://github.com/ome/omero-ms-zarr/commit/09d3103d098cfd9f95f5b6004f393439f4987844.

This raises the question of whether pre-commit should be executed in a phase of omero-test-infra or be fully delegated to a separate GitHub workflow. The former might simplify this change to its bare minimum and remove the requirement to extend the base Docker images. In all cases, I think some mechanism will be needed allowing to support both styles of plugins depending on the presence of .pre-commit-config.yaml.

manics commented 3 years ago

This PR already supports both, pre-commit is only run if .pre-commit-config.yaml is found, otherwise the old behaviour is retained.

sbesson commented 3 years ago

Agreed this PR supports both behaviors. I guess my thought that if we decided that pre-commit is not an omero-test-infra phase, I assume this PR could be limited to a simple no-op in the presence of .pre-commit-config.yaml, no?

manics commented 3 years ago

True, though git will still be needed for testing repositories that use setuptols-scm so in the long term it we still need a Docker image with some dev tools installed.

In either case I don't mind which route we follow for pre-commit, does someone want to make a decision?

joshmoore commented 3 years ago

To start getting PRs cleaned up, I'm going to go ahead and merge this as is. If we need to extract pre-commit (and flake) from omero-test-infra to simplify like, we can do that in a second round. Thanks all.