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

Add shellcheck to all repositories #289

Closed
4 of 5 tasks
sarayourfriend opened this issue Aug 11, 2022 · 11 comments
Closed
4 of 5 tasks

Add shellcheck to all repositories #289

sarayourfriend opened this issue Aug 11, 2022 · 11 comments
Assignees
Labels
🤖 aspect: dx Concerns developers' experience with the codebase ✨ goal: improvement Improvement to an existing user-facing feature good first issue New-contributor friendly help wanted Open to participation from the community 🟨 priority: medium Not blocking but should be addressed soon

Comments

@sarayourfriend
Copy link
Collaborator

sarayourfriend commented Aug 11, 2022

Problem

We've made a few easy to avoid mistakes in shell scripts

WordPress/openverse-frontend#1640
WordPress/openverse-api#869

These can be avoided using shellcheck and following it's basic advice (always adding set -e, actual syntax checks, etc).

Description

Add shellcheck to all repositories. Even if any of these repositories don't have shell scripts, let's add it now before we introduce a shell script and forget about this. It doesn't hurt, after all.

Side note

It'd be nice if we used pre-commit on all repositories instead of having the frontend repository be the odd ball using husky. If we used pre-commit everywhere then we could just sync the pre-commit configuration and automagically have things working for all languages across all repositories the same way. I'll open another issue to think about that for the frontend repo.

Implementation

  • 🙋 I would be interested in implementing this feature.
@sarayourfriend sarayourfriend added 🚦 status: awaiting triage Has not been triaged & therefore, not ready for work ✨ goal: improvement Improvement to an existing user-facing feature labels Aug 11, 2022
@dhruvkb
Copy link
Member

dhruvkb commented Aug 11, 2022

Since shellcheck has a readymade integration for pre-commit, I'm going to go ahead and mark this issue as good first.

@dhruvkb dhruvkb added good first issue New-contributor friendly help wanted Open to participation from the community 🟨 priority: medium Not blocking but should be addressed soon 🤖 aspect: dx Concerns developers' experience with the codebase and removed 🚦 status: awaiting triage Has not been triaged & therefore, not ready for work labels Aug 11, 2022
@MustkimKhatik
Copy link
Contributor

MustkimKhatik commented Sep 6, 2022

Hey, I would like to work on this. And I would appreciate if you tell, in which file of repository we are adding shell check?

@dhruvkb
Copy link
Member

dhruvkb commented Sep 6, 2022

Hey @MustkimKhatik, we want to add shell-check to all 4 of our repos. The Python-based repos all use pre-commit for linting, which is configured using the .pre-commit-config.yaml file in the root of the repos.

Repo File
openverse https://github.com/WordPress/openverse/blob/main/.pre-commit-config.yaml
openverse-catalog https://github.com/WordPress/openverse-catalog/blob/main/.pre-commit-config.yaml
openverse-api https://github.com/WordPress/openverse-api/blob/main/.pre-commit-config.yaml

Adding it to the frontend would be more contrived but you can start with these ^ three.

@MustkimKhatik
Copy link
Contributor

Alright! I would work on this three!

@MustkimKhatik
Copy link
Contributor

btw this pre-commit syntax will go as it is or there are any changes in args?

@dhruvkb
Copy link
Member

dhruvkb commented Sep 7, 2022

@MustkimKhatik I think we can skip the args field completely so that all the messages are shown.

@MustkimKhatik
Copy link
Contributor

@MustkimKhatik I think we can skip the args field completely so that all the messages are shown.

Alright we will do that

@MustkimKhatik
Copy link
Contributor

MustkimKhatik commented Oct 5, 2022

Hey @dhruvkb, For frontend, workaround is same? because there is no .pre-commit-config.yaml file so where we adding ShellCheck?

@sarayourfriend
Copy link
Collaborator Author

@MustkimKhatik We'd have to add it as an additional workflow job in the CI/CD for the frontend repository. This could be done with this action, for example: https://github.com/marketplace/actions/shellcheck

Alternatively, Dhruv has been working on getting pre-commit into the frontend repository in this PR: WordPress/openverse-frontend#1862 Maybe helping on that PR would be a good way to go about this as well. Getting pre-commit in the frontend would help unify our linting across all repositories.

@MustkimKhatik
Copy link
Contributor

Okay @sarayourfriend, maybe adding ShellCheck directly into pre-commit will be more convenient instead of additional workflow job. I would definitely look for it.

@dhruvkb
Copy link
Member

dhruvkb commented Oct 11, 2022

Closing this as all our active repos are now using pre-commit with a nearly identical config that includes ShellCheck.

@dhruvkb dhruvkb closed this as completed Oct 11, 2022
dhruvkb added a commit that referenced this issue Feb 20, 2023
dhruvkb pushed a commit that referenced this issue Apr 14, 2023
* Simplify loader workflow, move operator definitions into DAG

* Add more comments, skip on the first task rather than continue

* Change trigger rule on table drop

* There should really only be one of these at a time (for now)

This could change if this DAG is triggered automatically from the provider DAGs.

* Fix op kwarg, change DAG so drop table only runs after create table

* Update tests

* Add per-task documentation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖 aspect: dx Concerns developers' experience with the codebase ✨ goal: improvement Improvement to an existing user-facing feature good first issue New-contributor friendly help wanted Open to participation from the community 🟨 priority: medium Not blocking but should be addressed soon
Projects
None yet
Development

No branches or pull requests

3 participants