-
Notifications
You must be signed in to change notification settings - Fork 12
⚠️ CONFLICT! Lineage pull request for: skeleton #82
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
Conversation
Add rules to enforce ATX-closed headers, dashes for unordered list elements, and `1.` for ordered list elements.
Add <h1> and <img> tags to the allowed list for MD033 (HTML elements) to support using an image as the first thing in a markdown file (header image).
This converts the existing `.mdl_config.json` file to an equivalent `.mdl_config.yaml` file. The reference in the markdownlint pre-commit hook configuration is updated to match. Co-authored-by: Shane Frasier <jeremy.frasier@trio.dhs.gov>
Now that this is a YAML file we can add comments explaining the rule modifications we use. This will make it easier to edit or expand in the future.
This hook will validate any pre-commit hook manifest files in the repository.
We should be doing this because the Packer and Terraform pre-commit hooks leverage the corresponding executables; therefore, it makes sense to go ahead and install the particular versions of those executables that we support. Also add support for optionally debugging via tmate. See also cisagov/skeleton-generic#74.
Co-authored-by: Nick M. <50747025+mcdonnnj@users.noreply.github.com>
There is no reason to create /usr/bin/terraform. This is a vestige of an earlier age. Co-authored-by: Nick M. <50747025+mcdonnnj@users.noreply.github.com>
The Terraform installation does not destroy the existing system Terraform installation, and neither should the Packer installation. Co-authored-by: Nick M. <50747025+mcdonnnj@users.noreply.github.com>
Note that this change is dependent on the merging of cisagov/setup-env-github-action#31. Co-authored-by: Nick M. <50747025+mcdonnnj@users.noreply.github.com>
Some variables defined in the go installation are used in the cache task, so the go installation must happen first. Co-authored-by: Nick M. <50747025+mcdonnnj@users.noreply.github.com>
Co-authored-by: Nick M. <50747025+mcdonnnj@users.noreply.github.com>
As of [Go 1.16](https://tip.golang.org/doc/go1.16#go-command) the `GO111MODULE` environment variable defaults to `on` and `go get` has been deprecated for module installation. Co-authored-by: Nick M. <50747025+mcdonnnj@users.noreply.github.com>
The `shfmt` tool does not ship on the GitHub Actions runners so we must install it manually.
We have had a difficult time with how beautysh parses some shellscripts. I went in pursuit of an alternative and I believe shfmt to be a good alternative. Co-authored-by: Shane Frasier <jeremy.frasier@trio.dhs.gov>
This is performed by running `pre-commit autoupdate`, but with the `ansible-lint` hook held back manually.
Update pre-commit Hooks
Add the validate_manifest Hook to pre-commit Configuration
Update markdownlint Configuration
…-for-linting Install terraform and packer for the linting job
…_shfmt Replace beautysh hook with cisagov/pre-commit-shfmt
With the release of 0.900 mypy moved to a modular stub model. This means that only stdlib stubs are "baked in" and everything else must be installed separately. Since the mypy hook runs in a venv the stubs it recommends need to be installed as additional dependencies.
5e32a17
to
77c19cb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One item I thought of that we should resolve before merging this.
additional_dependencies: | ||
- types-setuptools |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also add these dependencies to requirements-test.txt
or requirements-dev.txt
so that mypy
also works from the developer's local venv?
If so, then is there any way to add a requirements-mypy.txt
, say, that can be pulled in by both requirements-test.txt
(or requirements-dev.txt
- I'm not certain where it should go) and our pre-commit
configuration file? I'd prefer to add these dependencies in only one place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'd be cleaner to leverage mypy --install-types --non-interactive src/
in setup-env
to automatically install them to the local environment as part of setup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since that also goes through type checking, we could eschew the --non-interactive
part (which will also force a user to interact with the setup-env
script during setup).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'd be cleaner to leverage
mypy --install-types --non-interactive src/
insetup-env
to automatically install them to the local environment as part of setup.
OK, that makes sense. That's a change we'd want to make upstream, in skeleton-generic right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since that also goes through type checking, we could eschew the
--non-interactive
part (which will also force a user to interact with thesetup-env
script during setup).
I don't understand what you're saying here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry the --non-interactive
step will also proceed with mypy
's type checking. Without that flag you have to do the --install-types
step and then run mypy
to do type checking. People wanted to do it all at once, but for our purposes it's ok to keep it broken up I believe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'd be cleaner to leverage
mypy --install-types --non-interactive src/
insetup-env
to automatically install them to the local environment as part of setup.OK, that makes sense. That's a change we'd want to make upstream, in skeleton-generic right?
Currently mypy
is only a development requirement for projects descending from this skeleton. We can either add it here or add it to cisagov/skeleton-generic and add mypy
as a requirement as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'd be cleaner to leverage
mypy --install-types --non-interactive src/
insetup-env
to automatically install them to the local environment as part of setup.OK, that makes sense. That's a change we'd want to make upstream, in skeleton-generic right?
Never mind, I see now that mypy
isn't even installed in skeleton-generic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See commit 3452b9d.
We already do this as part of the mypy pre-commit hook, but this way mypy is also ready to run manually in the developer's local Python virtual env.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it! 🏆
Lineage Pull Request: CONFLICT
Lineage has created this pull request to incorporate new changes found in an
upstream repository:
Upstream repository:
https://github.com/cisagov/skeleton-generic.git
Remote branch:
HEAD
Check the changes in this pull request to ensure they won't cause issues with
your project.
The
lineage/skeleton
branch has one or more unresolved merge conflictsthat you must resolve before merging this pull request!
How to resolve the conflicts
Take ownership of this pull request by removing any other assignees.
Clone the repository locally, and reapply the merge:
Review the changes displayed by the
status
command. Fix any conflicts andpossibly incorrect auto-merges.
After resolving each of the conflicts,
add
your changes to thebranch,
commit
, andpush
your changes:Note that you may append to the default merge commit message
that git creates for you, but please do not delete the existing
content. It provides useful information about the merge that is
being performed.
Wait for all the automated tests to pass.
Check the "Everything is cool" checkbox below:
Mark this draft pull request "Ready for review".
Note: You are seeing this because one of this repository's maintainers has
configured Lineage to open pull requests.
For more information:
🛠 Lineage configurations for this project are stored in
.github/lineage.yml
📚 Read more about Lineage