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

ci: Add pre-commit checks #53

Merged
merged 1 commit into from Oct 28, 2020
Merged

ci: Add pre-commit checks #53

merged 1 commit into from Oct 28, 2020

Conversation

ghost
Copy link

@ghost ghost commented Oct 23, 2020

Signed-off-by: david-doit-intl david@doit-intl.com

@ghost ghost requested a review from stepanstipl October 23, 2020 13:57
@ghost ghost force-pushed the feature/cliUpdate branch 3 times, most recently from 1537918 to 151f9db Compare October 23, 2020 14:06
Signed-off-by: david-doit-intl <david@doit-intl.com>
@ghost ghost assigned stepanstipl Oct 23, 2020
Copy link
Contributor

@stepanstipl stepanstipl left a comment

Choose a reason for hiding this comment

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

Thanks @david-doit-intl .

  1. The doc improvements are nice 👍

  2. The test coverage 👍 - it would be nice to expose this somehow more visibly and check that we're not getting worse with any single commit :)... but this is a good start.

  3. I have some concerns about the pre-commit func. In particular:

  • a) It expects/installs stuff on host system which I find a bit dangerous, and cluttering - ie. pre-commit is python, but will install go (such as checkmake). I didn't dig into this too much, but as someone who's trying to control what and in which way is installed on my system, this makes me somewhat uncomfortable. I expect this can "break" in many ways, depending on your OS and other bits.
  • b) Reproducibility of the builds - because of environment dependencies, and referencing master versions of those hook repos., we might get different results.
  • c) Quality and usefulness of the individual hooks - like for example the checkmake one doesn't seem to do much.

Overall it seems like a potentially complex system with a lot of dependencies.

Also re. the build reproducibility, I would like to keep easy parity of being able to execute all tests via make, both locally and in ci. There's a pattern, which I like (and would assume we have proper build image, currently golang one). This allows you to easily run any make step both locally and/or from inside the container), as well from outside by prefixing it with docker-.

docker-%:
	docker run -it --rm -e TF_VAR_project $(DOCKER_IMAGE) $*

Finally, I really like the idea of pre-commit (just not the implementation so much), and realised how tired am I of writing same CI bits over and over again :). In longer-term, I started looking into nix and it's shell functionality (check
https://github.com/hashicorp/waypoint/blob/main/shell.nix for an example how this can be used) as a better option to purely docker based reproducible dev/build environments. And considering building a shared library of "CI" scripts... but this might take a while.

So on a pragmatic note, the results are good -> let's merge this as it is 👍 and improve later :).

@stepanstipl stepanstipl merged commit 39de4e1 into master Oct 28, 2020
@stepanstipl stepanstipl deleted the feature/cliUpdate branch October 28, 2020 12:07
@stepanstipl stepanstipl changed the title Pre-commit ci: Add pre-commit checks Nov 11, 2020
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.

None yet

1 participant