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

ci: change the order of lint jobs as lint_shell is more likely to fail #2447

Merged
merged 1 commit into from
Aug 1, 2023
Merged

ci: change the order of lint jobs as lint_shell is more likely to fail #2447

merged 1 commit into from
Aug 1, 2023

Conversation

Henrik66
Copy link
Contributor

@Henrik66 Henrik66 commented Jul 26, 2023 •
edited
Loading

ci: change the order of lint jobs as lint_shell is more likely to fail..

Now the lint_shell test result is "above the scroll" on the page.

Screen Shot 2023-07-29 at 2 49 15 PM

@Henrik66 Henrik66 marked this pull request as draft July 26, 2023 12:39
@Henrik66
Copy link
Contributor Author

Henrik66 commented Jul 26, 2023 •
edited
Loading

Lint CI seems to pass as green even if sub-action step lint-shell is failing (red).

Screen Shot 2023-07-26 at 9 17 10 AM Screen Shot 2023-07-26 at 9 17 27 AM

@github-actions github-actions bot added the github Issues related to .github label Jul 26, 2023
@Henrik66
Copy link
Contributor Author

After PR CI is failing as expected

Screen Shot 2023-07-26 at 11 28 48 AM

@Henrik66 Henrik66 changed the title test shcellcheck ci: combine two jobs into one to avoid false positives Jul 26, 2023
@Henrik66 Henrik66 marked this pull request as ready for review July 26, 2023 15:30
LaszloGombos
LaszloGombos approved these changes Jul 26, 2023
View reviewed changes
Copy link
Collaborator

@LaszloGombos LaszloGombos left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Great find. Thanks.

Next time please consolidate your comments into the main PR message to help reviewers to understand what are you trying to solve.

aafeijoo-suse
aafeijoo-suse approved these changes Jul 27, 2023
View reviewed changes
Copy link
Member

@aafeijoo-suse aafeijoo-suse left a comment

Choose a reason for hiding this comment

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

Maybe this approach is fine, but AFAIK we were able to identify an entry with Lint / lint-shell so far, the problem is that both Lint entries are not ordered in the checklist.

@LaszloGombos
Copy link
Collaborator

Can not land for the same reason as

#2385 (comment)

Need to look into alternatives to help with the reviews and make the failure more obvious for other contributors who do not contribute to dracut regularly...

@Henrik66 Henrik66 changed the title ci: combine two jobs into one to avoid false positives ci: make sure both lint jobs succeed Jul 29, 2023
@Henrik66 Henrik66 force-pushed the shellcheck branch 2 times, most recently from 0194571 to e526be4 Compare July 29, 2023 03:44
@Henrik66
Copy link
Contributor Author

Screen Shot 2023-07-28 at 11 49 25 PM

I think I was wrong. Both job failures are reported without changing the code. It is just hard to find the failure for lint-shell as it is usually at the bottom.

@LaszloGombos What should I do now ? Should I close this PR ?

@LaszloGombos
Copy link
Collaborator

@Henrik66 For the goal of making lint-shell test result more easy to find (and run it earlier) perhaps we can at least change the order the test run. What do you think ?

@Henrik66 Henrik66 changed the title ci: make sure both lint jobs succeed ci: change the order of lint jobs as lint_shell is more likely to fail Jul 29, 2023
LaszloGombos
LaszloGombos approved these changes Jul 29, 2023
View reviewed changes
Copy link
Collaborator

@LaszloGombos LaszloGombos left a comment

Choose a reason for hiding this comment

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

Thanks @Henrik66 .

@aafeijoo-suse are you still ok with this changed PR to land ?

@aafeijoo-suse aafeijoo-suse merged commit 83eccc7 into dracutdevs:master Aug 1, 2023
72 of 81 checks passed
@Henrik66 Henrik66 deleted the shellcheck branch August 1, 2023 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LaszloGombos LaszloGombos LaszloGombos approved these changes

@aafeijoo-suse aafeijoo-suse aafeijoo-suse approved these changes

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

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

Assignees
No one assigned
Labels
github Issues related to .github
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Henrik66 @LaszloGombos @aafeijoo-suse