Code review comment for lp:~kyle-ireland/opencompute/Checkbox-Port-CentOS

Revision history for this message
Jeff Marcom (jeffmarcom) wrote :

Awesome work Kyle!
Now, I want to make sure we're handling this the correct way. Earlier, there was a merge that added a new package type to checkbox called "checkbox-ocp".

Our original discussion suggested that this proposed fix make its way into checkbox-cli, however I feel that this symlink check be instead now incorporated into "bin/checkbox-ocp" instead.

The fact that we have "bin/checkbox-ocp" will give you a bit more flexibility into making this work for other distributions without hindering the back-porting of features of lp:checkbox into lp:opencompute/checkbox.

This of course isn't your fault, and if it were not for the checkbox-ocp commit earlier today this would have a BIG stamp of approval from myself atleast.

Jeff Lane:

To give some context here (in case you look ) is that through the process of porting from debian to rpm using alien, the symlink "configs" is not preserved/made when installing with the rpm. Instead, Kyle has put a non-intrusive check to make sure it exists before executing checkbox.

There's one more thing,

I believe it should be if [ ! -s ] or [ ! -L] not if [ ! -S], please correct me if I'm wrong.

Thanks!!

review: Needs Fixing

« Back to merge proposal