Skip to content

hotfix: fix 701fdcd #91

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

Merged

Conversation

maryamtahhan
Copy link
Contributor

701fdcd disabled Markdown linting and intoduced
a different note context to the README.md. This
commit fixes that.

701fdcd disabled Markdown linting and intoduced
a different note context to the README.md. This
commit fixes that.

Signed-off-by: Maryam Tahhan <[email protected]>
@maryamtahhan
Copy link
Contributor Author

@SamYuan1990 @rootfs Can you please review?

@maryamtahhan maryamtahhan mentioned this pull request Jul 15, 2024
@SamYuan1990
Copy link
Contributor

yuanyi@yuanyideMacBook-Pro instructlab % docker pull markdownlint/markdownlint
Using default tag: latest
latest: Pulling from markdownlint/markdownlint
213ec9aee27d: Already exists 
5bedfe54446b: Retrying in 1 second 

the image still won't work.

@SamYuan1990
Copy link
Contributor

why not make it as a github action?

- name: markdownlint-cli2-action
  uses: DavidAnson/markdownlint-cli2-action@v9

@SamYuan1990
Copy link
Contributor

for example https://github.com/cncf/tag-env-sustainability/blob/main/.github/workflows/checks.yml#L21 sustainable tag lint markdown, as they using markdown as web page. but as markdown just our document should we so strict on markdown lint?

@dave-tucker
Copy link

If you install the pre-commit hooks it will perform the checks before you git commit
but yes, ideally we'd run the same checks in CI also

@SamYuan1990
Copy link
Contributor

If you install the pre-commit hooks it will perform the checks before you git commit but yes, ideally we'd run the same checks in CI also

I suppose if it's really needed, please put it into CI, otherwise, just checking for people who enabled with pre-commit, which still give chance for things get skipped.

@SamYuan1990
Copy link
Contributor

If you install the pre-commit hooks it will perform the checks before you git commit but yes, ideally we'd run the same checks in CI also

for example, if the container pull won't work for me, I would like to disable the pre commit check.

@maryamtahhan
Copy link
Contributor Author

maryamtahhan commented Jul 17, 2024

Hi @SamYuan1990

I can add a github workflow/action also.

what steps did you follow to see the failure?

On my mac I have docker running.
Then I cloned the repo.
in the repo I run:

python3 -m venv path/to/venv
source path/to/venv/bin/activate
pip install pre-commit
pre-commit run --all-files

I can see the pull working:

❯ pre-commit run --all-files
Check Yaml...............................................................Passed
Fix End of Files.........................................................Passed
Trim Trailing Whitespace.................................................Passed
markdownlint-cli2-rules-docker...........................................Passed
shellcheck...............................................................Passed
(venv)

I should add that if I try to commit a change and I've not sourced that python venv then pre-commit just doesn't run... so it shouldn't have an effect in envs where it's not installed/enabled to run

@SamYuan1990
Copy link
Contributor

Hi @SamYuan1990

I can add a git workflow also.

what steps did you follow to see the failure?

On my mac I have docker running. Then I cloned the repo. in the repo I run:

python3 -m venv path/to/venv
source path/to/venv/bin/activate
pip install pre-commit
pre-commit run --all-files

I can see the pull working:

❯ pre-commit run --all-files
Check Yaml...............................................................Passed
Fix End of Files.........................................................Passed
Trim Trailing Whitespace.................................................Passed
markdownlint-cli2-rules-docker...........................................Passed
shellcheck...............................................................Passed
(venv)

I should add that if I try to commit a change and I've not sourced that python venv then pre-commit just doesn't run... so it shouldn't have an effect in envs where it's not installed/enabled to run

I can't pull the docker image to my local.
hence, as the target is merge after check, int check on CI works well.
and a local version for pre commit hook causes version difference, as which relay on python venv, as the path I added in .gitignore. file.

To run lint on CI is better than a local pre commit hook, why we have to implements in pre commit hook way for lint?

@SamYuan1990
Copy link
Contributor

Hi @SamYuan1990

I can add a git workflow also.

what steps did you follow to see the failure?

On my mac I have docker running. Then I cloned the repo. in the repo I run:

python3 -m venv path/to/venv
source path/to/venv/bin/activate
pip install pre-commit
pre-commit run --all-files

I can see the pull working:

❯ pre-commit run --all-files
Check Yaml...............................................................Passed
Fix End of Files.........................................................Passed
Trim Trailing Whitespace.................................................Passed
markdownlint-cli2-rules-docker...........................................Passed
shellcheck...............................................................Passed
(venv)

I should add that if I try to commit a change and I've not sourced that python venv then pre-commit just doesn't run... so it shouldn't have an effect in envs where it's not installed/enabled to run

and meanwhile, I don't want it limit contributes, which

yuanyi@yuanyideMacBook-Pro local-dev-cluster % git commit -s
/Users/yuanyi/OpenSource/local-dev-cluster/bin/python3.12: No module named pre_commit

if someone doesn't install pre commit

or #92
for some one contributing from UI to help with a nit, in previously we just ask people to pass DCO, and for now.... he/she has to checkout code, config pre commit, have a container env for either podman or docker....

@maryamtahhan
Copy link
Contributor Author

for some one contributing from UI to help with a nit, in previously we just ask people to pass DCO, and for now.... he/she has to checkout code, config pre commit, have a container env for either podman or docker....

But a developer to this repo has these pre-requisites anyway:

These are listed in the Readme. I don't think pre-commit adds too much of an ask to install if they have already installed the pre-requisites.

Longer term it helps the quality of contributions. I can't replicate the issue you are seeing. And when I don't have pre-commit installed and I commit to the repo I don't see the error you are seeing either.

I'm happy to add a workflow also to ensure this is covered.

@SamYuan1990
Copy link
Contributor

SamYuan1990 commented Jul 17, 2024

make a summary here is my opinion:
lint is used to make code quality.
The implements for pre commit, adding risk for different local version as for Mac user, the pre commit which relays on venv, a n implements in CI is better way.

I hope we can make people easy to start with their contribution.

  • This repo works as a part of CI, well, we can't set a debugging point while github action is running. So we should learn how to fix code from CI log. Any failure is lesson and learn which we should add log to ensure we can debug without a break point.
  • For me, I am contribute in personal time, which usually 20:00 to 24:00, so I am working in an aync model, just submit the code see an overall errors and fix them in next commit. Which means, locally and historically to debug and development this Repo, I just ref CI logs to see how the code influences pipeline. As in self host runner, some local env as $home is not available.
  • So, instead of adding complexity on local environmental config or set up, I would like to see an alternative as pipeline based lint.

@SamYuan1990
Copy link
Contributor

SamYuan1990 commented Jul 17, 2024

for some one contributing from UI to help with a nit, in previously we just ask people to pass DCO, and for now.... he/she has to checkout code, config pre commit, have a container env for either podman or docker....

But a developer to this repo has these pre-requisites anyway:

These are listed in the Readme. I don't think pre-commit adds too much of an ask to install if they have already installed the pre-requisites.

Longer term it helps the quality of contributions. I can't replicate the issue you are seeing. And when I don't have pre-commit installed and I commit to the repo I don't see the error you are seeing either.

I'm happy to add a workflow also to ensure this is covered.

Honestly, I haven't use my local container run time for half years more....
reasoning and tricks as #91 (comment)

which the vscode (my personal IDE) as a webUI for cloud IDE, and CI plays role as remote server. :-)

@maryamtahhan
Copy link
Contributor Author

there's an option to skip the pre-commit hook if you are having issues with it that is to just pass --no-verify when you make your commit. example below:

tester:~/maryam/kepler# git add .
tester:~/maryam/kepler# git commit -m "test" --no-verify 
[precommit-shellcheck-mardown 89ed1adc] test
 1 file changed, 1 insertion(+), 1 deletion(-)

VS the normal flow:

tester:~/maryam/kepler# git add .
tester:~/maryam/kepler# git commit -m "test"
trim trailing whitespace.................................................Passed
fix end of files.........................................................Passed
check for added large files..............................................Passed
yamllint.............................................(no files to check)Skipped
clang-format.........................................(no files to check)Skipped
markdownlint-cli2-rules-docker.......................(no files to check)Skipped
codespell................................................................Passed
golangci-lint............................................................Passed
[precommit-shellcheck-mardown 82d24da4] test
 1 file changed, 1 insertion(+), 1 deletion(-)

@maryamtahhan maryamtahhan force-pushed the hotfix-markdown-linting branch from adef7a1 to b4c2b49 Compare July 18, 2024 15:06
Copy link
Contributor

@SamYuan1990 SamYuan1990 left a comment

Choose a reason for hiding this comment

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

LGTM, will test via another PR.

@SamYuan1990 SamYuan1990 merged commit 60ff8c9 into sustainable-computing-io:main Jul 21, 2024
7 checks passed
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.

3 participants