feat(lint): make testing lint easier #1753

georgettica · 2020-12-27T16:19:14Z

related to #1743

as I created Bash-it#1753 and seperated the dependencies, there is no need for the duplication This reverts [PARTIALLY] commit 026ccfe.

NoahGorny · 2020-12-27T17:27:04Z

I am unsure about this, as this seems like very opinionated change, but allows developers without all the setup to run the lint stage. However, this sort of thing should be handled by the person wanting to contribute, as people tend to have different opinions about this kind of stuff...

georgettica · 2020-12-27T17:50:30Z

I am fine with it being changed and generalized based on opinion.
If you would like we can add this to the bash-it container and use that one to test.

This way we can validate the scripts are ok, and make the container mode widely used

nwinkler · 2020-12-28T17:04:42Z

In general, I like the idea of running tools in containers, since it makes it easier to run uncommon tools without having to install all of the dependencies. The tradeoff is that you have to have Docker installed...

I'm also careful with using open-sourced containers, e.g. from Docker Hub, since many of them contain security vulnerabilities or malware - we need to be careful to not introduce these issues here. There are some pre-built shfmt containers out there, but we'd need to check whether they are up to date and reasonably secure. Rebuilding the container before running the command feels like a bit of a waste, ideally, we'd use a pre-built container that has all of the required dependencies.

To me, it feels like these instructions are a better fit for our documentation - I don't see us adding more and more test scripts to the project root, it will get crowded there...

davidpfarrell · 2020-12-28T19:57:20Z

Hi Team,

After grinding on this for a bit, here are my thoughts:

Create a bin/ folder for non-primary scripts (addresses crowded root folder)
Consider moving lint_clean_files.sh into bin/
Create an official bash-it-pre-commit-docker image
- It lives alongside our bash-it-docker image
- It contains a fully configured (as per Bash-It needs) pre-commit setup. ie:
  - pre-commit
  - shfmt
Create bin/pre-commit-docker.sh
- Knows how to invoke pre-commit from the docker image running against the current bash-it folder
Create 'bin/lint_clean_files-docker.sh'
- Knows how to invoke pre-comit-docker.sh against clean_files.txt

This would provide a basic ability to lint files without having to have pre-commit/shfmt installed locally.

The user could get clever and do things like:

alias pre-commit ~/.bashit/bin/pre-commit-docker.sh or
ln -s ~/.bashit/bin/pre-commit-docker.sh /usr/local/bin/pre-commit
Which would allow them to invoke lint_clean_files.sh directly for convenience.

georgettica · 2020-12-29T07:51:33Z

I am all for what @davidpfarrell said, and I agree with ya'll on the concerns.. should we create a roadmap we agree on for this?

I think that:

moving the container into the bash-it-container is a good idea
moving non-necessary scripts into ./bin is also a fair point
we could also add an env var check into 'clean-files.sh' to run the checks inside a container if need be

if [[ -n $RUN_IN_CONTAINER ]]; then
  ./lint_clean_files-docker.sh
  exit
fi

davidpfarrell · 2020-12-29T18:45:47Z

moving the container into the bash-it-container is a good idea

You know, my statement was actually to create a separate container for the pre-commit, ie:

docker pull bashit/pre-commit

Having it in the same container may also be useful, but I think there's value in a stand-alone container as well.

georgettica · 2020-12-30T08:14:44Z

created PR Bash-it/bash-it-docker#3 on it, we can do some stuff there and them move back to this one

cornfeedhobo · 2021-01-01T22:01:45Z

Howdy y'all. Great discussion! Here's my $0.02

there should be a dedicated container for pre-commit. It can be the base upon which the bash-it container is built, but it should be independent.
moving scripts to bin seems like an easy win, but we should still do everything possible to reduce sprawl.
keeping with the previous point, lint_clean_files.sh should be updated to check for an environment variable to toggle running in a container, and any docker-specific scripts should simply set that environment variable and execute lint_clean_files.sh, but I would caution against creating docker-specific scripts at all.
- this ensures that any time someone touches lint_clean_files.sh, they don't need to consider two files.
- also, if we add more docker support, each script won't need a corresponding docker version.
- note: I'm torn on this entire last point, so I could be convinced otherwise.

georgettica · 2021-01-04T22:04:22Z

Hey y'all!
I have the PR for the dockerfile written out and it fails for some reason

Can someone TAL to see what I missed so we can proceed with this PR?

as I created Bash-it#1753 and seperated the dependencies, there is no need for the duplication This reverts [PARTIALLY] commit 026ccfe.

georgettica · 2021-03-04T09:25:59Z

hey! bumping up as it didn't get eyes yet...

can you TAL at Bash-it/bash-it-docker#3 ?

NoahGorny

hey @georgettica, nice work here! I left comments here and in the second PR, you are welcome to take a look 😄

NoahGorny · 2021-03-04T20:07:23Z

containerized_lint.sh

@@ -0,0 +1,6 @@
+#!/bin/bash
+
+CONTAINER_ENGINE=${CONTAINER_ENGINE:-docker}


I would place this script somewhere in the docs, so ppl will know that they can use it

NoahGorny · 2021-03-04T20:07:32Z

containerized_lint.sh

@@ -0,0 +1,6 @@
+#!/bin/bash
+
+CONTAINER_ENGINE=${CONTAINER_ENGINE:-docker}


We should probably note that this variable exists and how to use it in the same doc

NoahGorny · 2021-03-04T20:08:04Z

containerized_lint.sh

+
+CONTAINER_ENGINE=${CONTAINER_ENGINE:-docker}
+
+${CONTAINER_ENGINE} build --file ./testing.Dockerfile --tag test .


no need to build the docker each time- lets just use one we will publish ourselves

NoahGorny · 2021-03-04T20:11:52Z

testing.Dockerfile

@@ -0,0 +1,12 @@
+FROM fedora


so should we use this dockerfile or the one in the other repo? this one is more minimal...

I vote that we will use this one, just put this in here

The reason I have 2 is this one is based on fedora and is a little heavier, and the other is an alpine with only the requirements.

But I am glad to move to this as it's my go-to container as of late

feat(lint): make testing lint easier

44b9941

related to Bash-it#1743

georgettica added a commit to georgettica/bash-it that referenced this pull request Dec 27, 2020

fix(lint): remove duplicate files

e5234f5

as I created Bash-it#1753 and seperated the dependencies, there is no need for the duplication This reverts [PARTIALLY] commit 026ccfe.

georgettica added a commit to georgettica/bash-it that referenced this pull request Dec 27, 2020

fix(lint): remove duplicate files

4a9e342

as I created Bash-it#1753 and seperated the dependencies, there is no need for the duplication This reverts [PARTIALLY] commit 026ccfe.

georgettica mentioned this pull request Dec 27, 2020

refactor(aliases): sort #1743

Merged

georgettica mentioned this pull request Dec 30, 2020

[WIP] init(dockerfile): add additional file Bash-it/bash-it-docker#3

Open

NoahGorny pushed a commit to georgettica/bash-it that referenced this pull request Jan 8, 2021

fix(lint): remove duplicate files

fdd0d6d

as I created Bash-it#1753 and seperated the dependencies, there is no need for the duplication This reverts [PARTIALLY] commit 026ccfe.

NoahGorny requested changes Mar 4, 2021

View reviewed changes

gaelicWizard mentioned this pull request Jan 18, 2022

BATS infra refactor #2031

Merged

6 tasks

seefood added the seems abandoned rattle the cage, and close if nobody wants to keep it going label Nov 7, 2024

seefood marked this pull request as draft December 28, 2024 19:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(lint): make testing lint easier #1753

feat(lint): make testing lint easier #1753

georgettica commented Dec 27, 2020

NoahGorny commented Dec 27, 2020

georgettica commented Dec 27, 2020

nwinkler commented Dec 28, 2020

davidpfarrell commented Dec 28, 2020

georgettica commented Dec 29, 2020

davidpfarrell commented Dec 29, 2020

georgettica commented Dec 30, 2020

cornfeedhobo commented Jan 1, 2021 •

edited

Loading

georgettica commented Jan 4, 2021

georgettica commented Mar 4, 2021

NoahGorny left a comment

NoahGorny Mar 4, 2021

NoahGorny Mar 4, 2021

NoahGorny Mar 4, 2021

NoahGorny Mar 4, 2021

georgettica Mar 5, 2021

		@@ -0,0 +1,6 @@
		#!/bin/bash

		CONTAINER_ENGINE=${CONTAINER_ENGINE:-docker}


		CONTAINER_ENGINE=${CONTAINER_ENGINE:-docker}

		${CONTAINER_ENGINE} build --file ./testing.Dockerfile --tag test .

feat(lint): make testing lint easier #1753

Are you sure you want to change the base?

feat(lint): make testing lint easier #1753

Conversation

georgettica commented Dec 27, 2020

NoahGorny commented Dec 27, 2020

georgettica commented Dec 27, 2020

nwinkler commented Dec 28, 2020

davidpfarrell commented Dec 28, 2020

georgettica commented Dec 29, 2020

davidpfarrell commented Dec 29, 2020

georgettica commented Dec 30, 2020

cornfeedhobo commented Jan 1, 2021 • edited Loading

georgettica commented Jan 4, 2021

georgettica commented Mar 4, 2021

NoahGorny left a comment

Choose a reason for hiding this comment

NoahGorny Mar 4, 2021

Choose a reason for hiding this comment

NoahGorny Mar 4, 2021

Choose a reason for hiding this comment

NoahGorny Mar 4, 2021

Choose a reason for hiding this comment

NoahGorny Mar 4, 2021

Choose a reason for hiding this comment

georgettica Mar 5, 2021

Choose a reason for hiding this comment

cornfeedhobo commented Jan 1, 2021 •

edited

Loading