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

add shfmt and shellcheck status checks #32

Closed
wants to merge 3 commits into from

Conversation

aaronhurt
Copy link
Owner

@aaronhurt aaronhurt commented Aug 23, 2024

This PR adds the shfmt and shellcheck status checks to improve code consistency and quality. Enforcing these checks will reduce variation and bugs in the codebase.

@aaronhurt
Copy link
Owner Author

@tschettervictor This is a work-in-progress branch for cleaning up the code and getting all the checks passing. While looking through I found some code that's clearly broken. I'll add line comments there as I'm not 100% sure what you were trying to do in those sections.

## run the check
if ! $REMOTE_CHECK > /dev/null 2>&1; then
exit_clean $? "Remote check '${REMOTE_CHECK}' failed!"
fi
}

## push replication function
do_push() {
Copy link
Owner Author

Choose a reason for hiding this comment

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

In the do_push function remote_set is never defined, but it's referenced. This will just be an empty string.

Copy link
Collaborator

@tschettervictor tschettervictor Aug 23, 2024

Choose a reason for hiding this comment

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

remote_set is always defined via the config.sh It gets calculated via the REPLICATE_SETS variable.

It is never altered as far as I know.

local source_snap="$($ZFS list -t snapshot -o name | grep ${dest_snap} 2> /dev/null | awk -F'@' '{print $2}' | tail -n 1)"
local common_snap="${local_set}@${dest_snap}"
local receiveargs="-vFd"
if [ "${1}" == "NULL" ] && [ ${RECURSE_CHILDREN} -eq 1 ]; then
Copy link
Owner Author

Choose a reason for hiding this comment

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

Let's also try to remove the generic $1, $2, etc. in the code and map them to local variables at the top of the function.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So, the purpose of the source and common snap was essentially to

  1. Check the destination for the latest snapshot
  2. Check if the corresponding snapshot exists on the source
  3. If it doesn't, then error out
  4. If it does exist, then do an incremental replication from one to the other

Copy link
Collaborator

Choose a reason for hiding this comment

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

How would we do this?

$1 = varname

or...?

Copy link
Owner Author

Choose a reason for hiding this comment

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

something like ...

local snapshot=$1

@aaronhurt
Copy link
Owner Author

aaronhurt commented Aug 23, 2024

If you could, I'd appreciate a second set of eyes and hands doing a line-by-line review of the code to ensure it functions as expected before we issue a new release. I have dinner plans this evening, but I might have some more time this weekend to do a bit more review.

@tschettervictor
Copy link
Collaborator

I can assure you each line has a purpose. I just have to get you to ask and I will tell. :)

Copy link
Collaborator

@tschettervictor tschettervictor left a comment

Choose a reason for hiding this comment

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

To be honest, most of this is deeper than the knowledge I have about bash.

It seems mostly ok, but have a look at some of the comments I gave.

readonly MODE_PULL="PULL"
readonly TYPE_REMOTE="REMOTE"
readonly TYPE_LOCAL="LOCAL"

Copy link
Collaborator

@tschettervictor tschettervictor Aug 24, 2024

Choose a reason for hiding this comment

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

I'm not sure why the Github actions wants to change the REMOTE and LOCAL to TYPE_REMOTE and TYPE_LOCAL. It makes no sense. The variable is set via the config.sh file. MODE_PULL is set to PULL anyway so...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also not really sure why it has to set the variables to readonly.

else
if ! do_push "NULL" ${local_set}@${sname} ${remote_set}; then
do_destroy ${local_set}@${sname}
exit_clean 99 ## TODO: use proper status
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why exit_clean here? If the replication fails for any reason, it should exit_error

if [ $scount -ge 1 ]; then
if ! do_pull ${base_snap} ${remote_set}@${sname} ${local_set}; then
do_destroy ${remote_set}@${sname}
exit_clean ## TODO: use proper status
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same issure with exit_clean here and for every function. The exit_clean should only be used when the replication completes successfully.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like it merged the exit codes into one block, so maybe it will work ok, but I don still want a FAILED when it fails the replication and SUCCESS when it completes.

@aaronhurt
Copy link
Owner Author

I'm going to abandon this PR but preserve the comments. I'm going to revert master back to the 0.7 release and then focus on the patch branch to get everything cleaned up for a release.

@aaronhurt aaronhurt closed this Aug 24, 2024
@tschettervictor
Copy link
Collaborator

Can we at least create a branch with the current script, to get to the ports repo?
The script function fine as is, I tested in in all scenarios.

Then we can work on code cleanup.

@aaronhurt aaronhurt deleted the ahurt/add-status-checks branch September 1, 2024 18:21
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

2 participants