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

Commenting out unused variables for shellcheck compliance. #256

Merged
merged 2 commits into from
Dec 29, 2022

Conversation

crazzy
Copy link

@crazzy crazzy commented Dec 15, 2022

Found this role being used in a project I'm collaborating on, where I want to run shellcheck on every single shellscript, so figured I'd patch this one and submit a PR.

Copy link
Owner

@evrardjp evrardjp left a comment

Choose a reason for hiding this comment

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

Thanks for the review. Good catch.

files/keepalived_notifications.sh Outdated Show resolved Hide resolved
@evrardjp
Copy link
Owner

Sorry for the late answer.

You can see the merge wasn't done due to failure in CI.

Here is the PR of the CI change. #259 .

When this is merged, I will rebase your code and run it again. If all goes well, it will be merged this week.

@evrardjp evrardjp dismissed their stale review December 29, 2022 09:30

Code was changed, change request should be dismissed

@evrardjp evrardjp added the bug Confirmed valid bug label Dec 29, 2022
@evrardjp
Copy link
Owner

evrardjp commented Dec 29, 2022

I just noticed that shellcheck isn't included in the tests, would you be okay to add it? Probably need to add somethign on the runners (.github/workflow/) and in the tox.ini (the shellcheck invocation).

Edit: We can do so in a follow up PR.

@evrardjp evrardjp added the enhancement Proposed addition to ansible-keepalived label Dec 29, 2022
@evrardjp evrardjp merged commit 66d4d0e into evrardjp:master Dec 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed valid bug enhancement Proposed addition to ansible-keepalived
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants