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

Fix disk state evaluation and add the shebang line #31

Merged
merged 2 commits into from Jun 8, 2022

Conversation

Doomas
Copy link
Contributor

@Doomas Doomas commented Jun 3, 2022

Hi

Disk state fix:
adding the disk states with += results in an Array of single letters. Therefore the final disk state was in every case "UNKOWN". With states.append() it works as intended.

Shebang:
The script was not automatically execute on my servers without the shebang

Regards Thomas

@wernerfred
Copy link
Owner

Change was introduced in #28 by @amotl - can you have a look too please?

@Doomas
Copy link
Contributor Author

Doomas commented Jun 3, 2022

Ok, I've checked it, and I think this a bug.

To append a entire String to an array you should use states.append()(I think this is the faster operation) or states += ["STATE"] (with square brackets).

But I like to know if this works for @amotl

@Kraeutergarten
Copy link

I can confirm this BUG. Please use append and add a python3 shebang.

Copy link
Owner

@wernerfred wernerfred left a comment

Choose a reason for hiding this comment

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

LGTM - @amotl pls share your thoughts on this anyways (going to merge in the next days if no response)

@wernerfred
Copy link
Owner

@all-contributors add @Doomas for code

@allcontributors
Copy link
Contributor

@wernerfred

I've put up a pull request to add @Doomas! 🎉

@wernerfred
Copy link
Owner

@all-contributors please add @Kraeutergarten for User Testing

@allcontributors
Copy link
Contributor

@wernerfred

I've put up a pull request to add @Kraeutergarten! 🎉

@amotl
Copy link
Contributor

amotl commented Jun 7, 2022

Hi there,

it definitively looks like that I made a mistake here. Apologies. Thank you very much for discovering and fixing this, @Doomas!

@wernerfred: I think it is ready to be merged. Go ahead!

With kind regards,
Andreas.

@wernerfred wernerfred merged commit 3808928 into wernerfred:master Jun 8, 2022
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.

None yet

4 participants