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

TEMPLATES/mega-linter.yml: DRY the ifs #2957

Merged
merged 8 commits into from
Oct 21, 2023
Merged

Conversation

rasa
Copy link
Contributor

@rasa rasa commented Sep 14, 2023

Proposed Changes

This simplifies TEMPLATES/mega-linter.yml by moving common code to its own step. This also can help with debugging, as these variables now show up in the logs. Accept or reject as you see fit.

Readiness Checklist

Edit: I've had success running this PR in private repos.

Author/Contributor

  • Add entry to the CHANGELOG listing the change and linking to the corresponding issue (if appropriate)
  • If documentation is needed for this change, has that been included in this pull request

@rasa rasa requested a review from nvuillam as a code owner September 14, 2023 18:05
@rasa rasa temporarily deployed to dev September 14, 2023 18:05 — with GitHub Actions Inactive
@rasa rasa temporarily deployed to dev September 14, 2023 18:05 — with GitHub Actions Inactive
Copy link
Member

@nvuillam nvuillam left a comment

Choose a reason for hiding this comment

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

@rasa it seems an interesting PR :)
Did you test it and do you confirm the behavior is exactly the same ? I wouldn't like to take the risk of regressions ^^

@github-actions
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had recent activity.
It will be closed in 14 days if no further activity occurs.
Thank you for your contributions.

If you think this pull request should stay open, please remove the O: stale 🤖 label or comment on the pull request.

@github-actions github-actions bot added the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Oct 19, 2023
@rasa
Copy link
Contributor Author

rasa commented Oct 19, 2023

Did you test it and do you confirm the behavior is exactly the same ? I wouldn't like to take the risk of regressions ^^

I have tested it locally, and both code paths work as intended. Unfortunately, I can't share logs as they are on private repos.

But since it's just DRYing the code, feel free to reject. I won't take offense.

@github-actions github-actions bot removed the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Oct 20, 2023
@nvuillam
Copy link
Member

@rasa I love DRY rules, so if you tested it let's trust you and see how it goes on own MegaLinter :)

But to do that, please can you also update own MegaLinter workflows ?

  • .github/workflows/mega-linter.yml
  • .github/workflows/mega-linter-for-runner.yml

@nvuillam
Copy link
Member

@rasa seems good but you need to merge main in your branch to solve conflicts before i can validate :)

@rasa rasa temporarily deployed to dev October 20, 2023 22:35 — with GitHub Actions Inactive
@rasa rasa temporarily deployed to dev October 20, 2023 22:35 — with GitHub Actions Inactive
@rasa rasa temporarily deployed to dev October 21, 2023 00:10 — with GitHub Actions Inactive
@rasa rasa temporarily deployed to dev October 21, 2023 04:04 — with GitHub Actions Inactive
Copy link
Member

@nvuillam nvuillam left a comment

Choose a reason for hiding this comment

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

Wonderful:)
Many thanks for your contribution @rasa 🎉

@nvuillam nvuillam merged commit 13aa8b5 into oxsecurity:main Oct 21, 2023
7 checks passed
@nvuillam
Copy link
Member

@rasa please can you also make a PR to update https://github.com/oxsecurity/megalinter/blob/main/mega-linter-runner/generators/mega-linter/templates/mega-linter.yml ?

This is the file that is used by the installer ^^

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.

2 participants