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

docs: Improve check_sample examples, add general comments #1039

Merged
merged 3 commits into from
Feb 10, 2022

Conversation

lazize
Copy link
Contributor

@lazize lazize commented Feb 9, 2022

Context

A lot of checks are failing due to the lack of error handling.
Also some best practices are being required on PR revision.

Description

Improve examples to fit them into the best practice and to pass most of ShellCheck.
Add general comments about what/how to do.

License

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@lazize lazize requested review from a team, toniblyx and jfagoagas February 9, 2022 20:58
@lazize
Copy link
Contributor Author

lazize commented Feb 9, 2022

Please check if General Comments make sense!

Copy link
Member

@jfagoagas jfagoagas left a comment

Choose a reason for hiding this comment

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

You raise a good point @lazize, thank you!!

Please, check my suggestions it could be useful for new contributors.

@@ -36,38 +36,68 @@
# CHECK_DOC_checkN=""
# CHECK_CAF_EPIC_checkN=""

# General comments
# ----------------
# Do not add double quotes (") arround variable ${PROFILE_OPT}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Do not add double quotes (") arround variable ${PROFILE_OPT}
# Do not add double quotes (") arround variable ${PROFILE_OPT} because this variable holds "--profile <profile-name>" and we need to read it as it is.

# - ${var:N} : Return string from position 'N'
# - ${var:N:len} : Return 'len' characters from position 'N'
# - ${var^^} : Convert to upper-case all characters
# - ${var,,} : Convert to lower-case all characters
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# - ${var,,} : Convert to lower-case all characters
# - ${var,,} : Convert to lower-case all characters
# - For more examples and how to use it please refer to https://www.gnu.org/software/bash/manual/bash.html#Shell-Parameter-Expansion

@jfagoagas jfagoagas added the status/waiting-for-revision Waiting for maintainer's revision label Feb 10, 2022
Copy link
Member

@jfagoagas jfagoagas left a comment

Choose a reason for hiding this comment

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

Thank you @lazize !!

@jfagoagas jfagoagas merged commit e8848ca into prowler-cloud:master Feb 10, 2022
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/waiting-for-revision Waiting for maintainer's revision
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants