Fix: Repo with no images gets SVN error

[Lewiscowles1986]

Fixes #57

Description of the Change

Guards image svn commands using glob check for images

Alternate Designs

Nothing else was considered

Benefits

No errors for plugins without the media. Allowing more basic and hobbyist devs to dip a toe in and gradually enhance plugin repo's.

Possible Drawbacks

I Hope not any from a technical standpoint.

Verification Process

I Used this code on one of my own repo's/

Checklist:

• I have read the CONTRIBUTING document.
• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my change.
• All new and existing tests passed.

Applicable Issues

#57

Changelog Entry

Changed

• Image upload no longer makes action fail if no images are in the repository being submitted.

[iamdharmesh]

Hi @Lewiscowles1986, Thanks a lot for the PR.

I think checking for files by extensions is a better approach here, because with this check if the repo has only .png images then it will still show the warning for command related to .jpg and .gif as the repo is not containing those files.
So, I think we can do something like below to check for files with specific extensions and run the command only if file is present. what do you think?

if test -d $ASSETS_DIR && test -n "$(find $ASSETS_DIR -maxdepth 1 -name *.png -print -quit)"; then
	svn propset svn:mime-type image/png assets/*.png || true
fi
if test -d $ASSETS_DIR && test -n "$(find $ASSETS_DIR -maxdepth 1 -name *.jpg -print -quit)"; then
	svn propset svn:mime-type image/jpeg assets/*.jpg || true
fi
if test -d $ASSETS_DIR && test -n "$(find $ASSETS_DIR -maxdepth 1 -name *.gif -print -quit)"; then
	svn propset svn:mime-type image/gif assets/*.gif || true
fi
if test -d $ASSETS_DIR && test -n "$(find $ASSETS_DIR -maxdepth 1 -name *.svg -print -quit)"; then
	svn propset svn:mime-type image/svg+xml assets/*.svg || true
fi

Thanks.

[dinhtungdu]

Can we also address the change introduced in #78 here?

[Lewiscowles1986]

Got a notification that shellcheck failed, so addressed it's feedback to quote inputs. Also quoted the mimes and globs.

[Lewiscowles1986]

Checked manually. It now passes shellcheck.

[iamdharmesh]

LGTM! Thanks.

[dinhtungdu]

I'm wondering why we need to check the $ASSETS_DIR here, while, inside the condition, we update mime type for images in the assets folder? Can we check the assets folder directly? And because the assets folder always exists, we only need to check if it contains images.

Suggested change

	if test -d "$ASSETS_DIR" && test -n "$(find "$ASSETS_DIR" -maxdepth 1 -name "*.png" -print -quit)"; then
	if test -n "$(find assets/ -maxdepth 1 -name "*.png" -print -quit)"; then

[Lewiscowles1986]

It does not always exist and this is the very reason for the creation of the issue and PR in the first place.

People should be able to have CI for repo's where they don't ship images or assets with their plugins.

[dinhtungdu]

@Lewiscowles1986 I'm talking about the assets folder checkout from svn here. That folder may not contain any file, but it always exists.

[Lewiscowles1986]

When I tried that on my computer it threw the following error.

find: Expected a positive decimal integer argument to -maxdepth, but got ‘-1’

[dinhtungdu]

@Lewiscowles1986 Which command did you use for testing? Which line throws that error?

What I really mean when I chimp in here is: we're checking if image files exist in one folder, then we set the mime type for images in a different folder => theoretically, it doesn't sound right to me.

I agree that the current implementation works as expected and possibly never has any issue, the asset files if exist may always be copied from $ASSETS_DIR to $SVN_DIR/assets using rsync. But if rsync fails to copy files in an edge case, our command will fail. Also, if in the future, we add a feature that may prevent copying the assets, we will need to update this again.

Alternatively, to be extra careful, we can use something like this:

if test -d "$SVN_DIR/assets" && test -n "$(find "$SVN_DIR/assets" -maxdepth 1 -name "*.png" -print -quit)"; then
    svn propset svn:mime-type "image/png" "$SVN_DIR/assets/*.png" || true
fi

Again, this is subtle and optional. Given that the current implementation is working fine, I leave the final decision to you and @iamdharmesh

[Lewiscowles1986]

Makes more sense now. Thank you for taking the time. I could adjust it to point at the svn folder, but in such a scenario would we not also consider if svn might fail to add the files? It's a piece of string I was not going to tug at. @iamdharmesh any chance you might cast a 👍 or 👎 on needing this?

[iamdharmesh]

we're checking if image files exist in one folder, then we set the mime type for images in a different folder => theoretically, it doesn't sound right to me.

Thanks for the note @dinhtungdu. I completely agree with this, we should check for images in the same directory on which we are going to run the mime-type command. Also, @Lewiscowles1986 after considering the edge case given by @dinhtungdu, checking for images in the SVN assets directory looks more proper solution to me.

Thanks

[Lewiscowles1986]

Right updated, squashed, pushed.

[dinhtungdu]

LGTM 👍🏽

[iamdharmesh]

LGTM. Thanks!!