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

differential-shellcheck pre-commit #1

Merged
merged 5 commits into from
May 6, 2024
Merged

Conversation

mpoberezhniy
Copy link
Owner

@mpoberezhniy mpoberezhniy commented Apr 28, 2024

Issue: redhat-plumbers-in-action/differential-shellcheck#234

TODO:

@mpoberezhniy
Copy link
Owner Author

mpoberezhniy commented Apr 28, 2024

Testing.
A docker image can be pulled and used as follows.

sudo docker run -t --rm --user="$(id -u):$(id -g)" --workdir=/src -v "$PWD":/src:rw,Z ghcr.io/mpoberezhniy/differential-shellcheck-precommit:v0.0.1

README.md.template Outdated Show resolved Hide resolved

This repo keeps the pre-commit hook out of the critical path of differential-shellcheck
releases, reducing the number of things that can go wrong. This in turn helps
ensure a smoother `pre-commit autoupdate`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a pre-commit user, but I think you are specifying the exact version you are using, so unless you update to a potentially broken version, everything should work.

I'm not against having a separate repo. On the other hand, having it in one repo would make it easier to add a set of tests that could prevent the pre-commit hook from breaking. But there would have to be certainly some level of cooperation either way :-)

Copy link
Owner Author

Choose a reason for hiding this comment

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

As of my pre-commit autoupdate code understanding, it pulls a repo copy locally (a precommit repo to be specific), iterates over tags, and selects the newest version to use. I think if this repo has proper actions to build release images - it should be stable.

I am open to merging this repository to differential-shellcheck or moving it somewhere else.
Thinking of the single responsibility principle we can keep them separate, I think I will have to implement an analog of src/summary.sh to get the output more readable in a terminal. This should allow not implementing redundant flags to control output styling and not adding the logic to stash/pop changes instead of checking out base/head commits.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that some work needs to be done to make differential shellcheck accessible from outside of GitHub Actions. My ultimate goal would be to make it possible to package it into distributions for local work.

I want to thank you for working on pre-commit; it's awesome. I will leave you the choice of where to have it (pre-commit code). :-)

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think I will be able to refactor it to have pre-commit and post-receive modes so pre-commit will be able to use it and the action will use it in a post-receive mode. This should help to get rid of the whole pre-commit.sh file and handling everything in index.sh while keeping the code readable.

If we have it as hooks - this should allow other SCM users to define their server-side hooks to trigger CI/CD and lint differentially.

@mpoberezhniy mpoberezhniy force-pushed the initial-implementation branch from d135077 to 544afa6 Compare April 29, 2024 14:23
@mpoberezhniy mpoberezhniy force-pushed the initial-implementation branch from 2fc0840 to 1c7171e Compare May 6, 2024 12:48
@mpoberezhniy mpoberezhniy force-pushed the initial-implementation branch from 1c7171e to 08451eb Compare May 6, 2024 14:11
@mpoberezhniy mpoberezhniy force-pushed the initial-implementation branch from 9a5b330 to 2717a4f Compare May 6, 2024 14:43
@mpoberezhniy mpoberezhniy marked this pull request as ready for review May 6, 2024 14:44
@mpoberezhniy
Copy link
Owner Author

Disregarding the rest of todo since planning to get this into differential-shellcheck once it supports cli arguments.

@mpoberezhniy mpoberezhniy merged commit eb5dd6b into main May 6, 2024
@mpoberezhniy mpoberezhniy deleted the initial-implementation branch May 6, 2024 14:51
@jamacku
Copy link
Contributor

jamacku commented May 6, 2024

@mpoberezhniy I have already started working on CLI, but I have it only locally. As soon as it takes shape, I'll create a draft PR.

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