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.

Sign up for GitHub

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

Jump to bottom

Adding pre-commit workflow #80

Merged
merged 2 commits into from
Jun 6, 2023
Merged

Adding pre-commit workflow #80

merged 2 commits into from
Jun 6, 2023

Conversation

Yarboa
Copy link
Collaborator

@Yarboa Yarboa commented May 31, 2023

Resolve #79

Add bash scan with shellcheck
Markdown scan with markdownlint-cli

@Yarboa Yarboa requested review from rhatdan, lsm5 and dougsland as code owners May 31, 2023 14:30
@Yarboa Yarboa marked this pull request as draft May 31, 2023 14:31
@Yarboa Yarboa force-pushed the adding-precommit branch from b8730b1 to 32aebc0 Compare May 31, 2023 14:32
@lsm5
Copy link
Member

lsm5 commented May 31, 2023

do these get listed as additional tasks or happen silently in the background?

dougsland
dougsland reviewed May 31, 2023
View reviewed changes
tests/e2e/run-test-e2e Outdated
@@ -128,7 +128,6 @@ while [[ $# -gt 0 ]]; do

--help)
usage
shift 2
Copy link
Collaborator

Choose a reason for hiding this comment

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

not seems related to this patch right? do you mind to open specific pr for this one? Otherwise looks good to me.
Could you also add flake8 validation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep,

@Yarboa Yarboa self-assigned this May 31, 2023
@Yarboa
Copy link
Collaborator Author

Yarboa commented Jun 1, 2023 •
edited
Loading

do these get listed as additional tasks or happen silently in the background?

I think the problem is this
https://github.com/containers/qm/pull/80/files#diff-3e103440521ada06efd263ae09b259e5507e4b8f7408308dc227621ad9efa31eR21

It is reject the event and never activate new pipeline
I will run some changes in e2e.yml also

@Yarboa Yarboa force-pushed the adding-precommit branch 3 times, most recently from e3b04b7 to affb8e7 Compare June 1, 2023 10:24
@rhatdan
Copy link
Member

rhatdan commented Jun 1, 2023

@lsm5 @dougsland PTAL

@lsm5
Copy link
Member

lsm5 commented Jun 2, 2023

do these get listed as additional tasks or happen silently in the background?

I think the problem is this https://github.com/containers/qm/pull/80/files#diff-3e103440521ada06efd263ae09b259e5507e4b8f7408308dc227621ad9efa31eR21

It is reject the event and never activate new pipeline I will run some changes in e2e.yml also

@Yarboa Do the e2e tests need to be run in github actions if they are being run with packit + TMT already? Or do these involve a different set of e2e tests?

@Yarboa
Copy link
Collaborator Author

Yarboa commented Jun 4, 2023

do these get listed as additional tasks or happen silently in the background?

I think the problem is this https://github.com/containers/qm/pull/80/files#diff-3e103440521ada06efd263ae09b259e5507e4b8f7408308dc227621ad9efa31eR21
It is reject the event and never activate new pipeline I will run some changes in e2e.yml also

@Yarboa Do the e2e tests need to be run in github actions if they are being run with packit + TMT already? Or do these involve a different set of e2e tests?

I thought of running precommit for shellcheck markdown and flake8
I think it could run on a container,
The 2nd option is to run TMT test, I think it is an overkill,

@Yarboa Yarboa force-pushed the adding-precommit branch from affb8e7 to cc343d6 Compare June 4, 2023 12:34
@Yarboa
Copy link
Collaborator Author

Yarboa commented Jun 5, 2023

do these get listed as additional tasks or happen silently in the background?

I think the problem is this https://github.com/containers/qm/pull/80/files#diff-3e103440521ada06efd263ae09b259e5507e4b8f7408308dc227621ad9efa31eR21
It is reject the event and never activate new pipeline I will run some changes in e2e.yml also

@Yarboa Do the e2e tests need to be run in github actions if they are being run with packit + TMT already? Or do these involve a different set of e2e tests?

I thought of running precommit for shellcheck markdown and flake8 I think it could run on a container, The 2nd option is to run TMT test, I think it is an overkill,

This one is working, on my private repo
https://github.com/Yarboa/qm/actions/runs/5175369846/jobs/9322872417

@Yarboa Yarboa marked this pull request as ready for review June 5, 2023 09:13
@Yarboa Yarboa marked this pull request as draft June 5, 2023 09:14
@Yarboa Yarboa force-pushed the adding-precommit branch from cc343d6 to b166f42 Compare June 5, 2023 10:24
@dougsland
Copy link
Collaborator

to me looks good, @lsm5 could you please review this one?
@Yarboa should we switch from draft to ready for review?

@rhatdan rhatdan marked this pull request as ready for review June 5, 2023 14:41
@Yarboa Yarboa force-pushed the adding-precommit branch 2 times, most recently from d131f66 to bf30823 Compare June 5, 2023 20:16
Yarboa added 2 commits June 5, 2023 23:18
@Yarboa
Adding pre-commit workflow
4c6e2d3 
Resolve containers#79
[Fix] added non relevant action e2e, removed

Signed-off-by: Yariv Rachmani <[email protected]>
@Yarboa
Fixing README.md
a53813d 
Fixing linting due to precommit ablaysis

Signed-off-by: Yariv Rachmani <[email protected]>
@Yarboa Yarboa force-pushed the adding-precommit branch from bf30823 to a53813d Compare June 5, 2023 20:20
@dougsland
Copy link
Collaborator

all of us already reviewed this one, let's make this fly.

@dougsland dougsland merged commit 4a022c7 into containers:main Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dougsland dougsland dougsland left review comments

@rhatdan rhatdan Awaiting requested review from rhatdan rhatdan is a code owner

@lsm5 lsm5 Awaiting requested review from lsm5 lsm5 is a code owner

Assignees

@Yarboa Yarboa

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add pre-commit shellcheck
4 participants
@Yarboa @lsm5 @rhatdan @dougsland