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 sandbox default solidus branch #192

Conversation

RyanofWoods
Copy link
Contributor

Fixes the sandbox.tt file script not defaulting to the master Solidus when no SOLIDUS_BRANCH variable is given.

Summary

When an uninitialized variable is given to -n, it is treated as not
NULL. The variable must be quoted for correct results.
[ -n "$SOLIDUS_BRANCH" ]

It is also recommended for variable comparisons in general as it can
produce incorrect results for ! -z.

References:

This meant that before, when running this file with no SOLIDUS_BRANCH
variable, $BRANCH would end up NULL and the branch in the Gemfile would
be an empty string. This caused an error, whereas instead, it should
have defaulted to master.

Error:

fatal: Needed a single revision
Git error: command `git rev-parse --verify ''` in directory /Users/ryanwoods/.rbenv/versions/2.7.6/lib/ruby/gems/2.7.0/cache/bundler/git/solidus-169f1ecb1aee2122950e6d586daf2410f62df66e has
failed.
Revision  does not exist in the repository https://github.com/solidusio/solidus.git. Maybe you misspelled it?
If this error persists you could try removing the cache directory
'/Users/ryanwoods/.rbenv/versions/2.7.6/lib/ruby/gems/2.7.0/cache/bundler/git/solidus-169f1ecb1aee2122950e6d586daf2410f62df66e'

The git source https://github.com/solidusio/solidus.git is not yet checked out. Please run `bundle install` before trying to start your application

Checklist

  • I have structured the commits for clarity and conciseness.
  • I have added relevant automated tests for this change.

To test this, I commented out half of the sandbox.tt file after (extension_name="<%= file_name %>") and ran:

bash lib/solidus_dev_support/templates/extension/bin/sandbox.tt

It logged that the master branch was being used and solidus_frontend and no debug mode was used.

I then ran:

DEBUG=true SOLIDUS_BRANCH="v3.2" SOLIDUS_FRONTEND="solidus_starter_frontend" bash lib/solidus_dev_support/templates/extension/bin/sandbox.tt

Which logged that the v.3.2 Solidus branch was being used and solidus_starter_frontend for the frontend 👍 It also correctly used the debug mode.
Screenshot 2022-11-09 at 10 19 50

When an uninitialized variable is given to `-n`, it is treated as not
NULL. The variable must be quoted for correct results.
`[ -n "$SOLIDUS_BRANCH" ]`

It is also recommended for variable comparisons in general as it can
produce incorrect results for `! -z`.

References:
- https://tldp.org/LDP/abs/html/comparison-ops.html
- https://tldp.org/LDP/abs/html/comparison-ops.html#STRTEST

This meant that before, when running this file with no SOLIDUS_BRANCH
variable, $BRANCH would end up NULL and the branch in the Gemfile would
be an empty string. This caused an error, whereas instead, it should
have defaulted to `master`.

Error:
```
fatal: Needed a single revision
Git error: command `git rev-parse --verify ''` in directory /Users/ryanwoods/.rbenv/versions/2.7.6/lib/ruby/gems/2.7.0/cache/bundler/git/solidus-169f1ecb1aee2122950e6d586daf2410f62df66e has
failed.
Revision  does not exist in the repository https://github.com/solidusio/solidus.git. Maybe you misspelled it?
If this error persists you could try removing the cache directory
'/Users/ryanwoods/.rbenv/versions/2.7.6/lib/ruby/gems/2.7.0/cache/bundler/git/solidus-169f1ecb1aee2122950e6d586daf2410f62df66e'

The git source https://github.com/solidusio/solidus.git is not yet checked out. Please run `bundle install` before trying to start your application
```
-z
string is null, that is, has zero length

-n
string is not null.

Reference:
- https://tldp.org/LDP/abs/html/comparison-ops.html
@mergify mergify bot added the needs changelog label Needs a label to determine the type of change. label Nov 9, 2022
@mergify
Copy link
Contributor

mergify bot commented Nov 9, 2022

It looks like this PR is missing a label to determine the type of change it introduces. The maintainer should add one of the following labels:

  • bug for bugfixes.
  • enhancement for new features and improvements.
  • documentation for documentation changes.
  • security for security patches.
  • removed for feature removals.
  • infrastructure for internal changes that should not go in the changelog.

Additionally, the maintainer may also want to add one of the following:

  • breaking for breaking changes.
  • deprecated for feature deprecations.

Once the correct labels have been set, simply remove the needs changelog label label from this PR so I can merge it.

@RyanofWoods
Copy link
Contributor Author

RyanofWoods commented Nov 9, 2022

It would probably be good to update some extension's sandbox script after this change. Note, I may soon make another PR to change the sandbox.tt further, so we may want to wait 👍


As another note, from my understanding, each extension copies the sandbox.tt file, but maybe it would reduce overhead, if extensions could just delegate to using the solidus_dev_support sandbox script if it doesn't need to modify it, so it doesn't need updating if changes are made to the main one. 🤔 On the other hand, changes to the main one could break the extension's sandbox setup.

@RyanofWoods
Copy link
Contributor Author

I am unable to change the label @waiting-for-dev

@waiting-for-dev waiting-for-dev added bug Describes or fixes a bug. and removed needs changelog label Needs a label to determine the type of change. labels Nov 9, 2022
Copy link
Contributor

@waiting-for-dev waiting-for-dev left a comment

Choose a reason for hiding this comment

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

Thanks for fixing it, @RyanofWoods!

@waiting-for-dev waiting-for-dev requested a review from elia November 9, 2022 12:12
@waiting-for-dev
Copy link
Contributor

As another note, from my understanding, each extension copies the sandbox.tt file, but maybe it would reduce overhead, if extensions could just delegate to using the solidus_dev_support sandbox script if it doesn't need to modify it, so it doesn't need updating if changes are made to the main one. 🤔 On the other hand, changes to the main one could break the extension's sandbox setup.

Yeah, at this point, the purpose of the sandbox application is to be used as a starting point, so I wouldn't change anything for now.

@mergify mergify bot merged commit 787b596 into solidusio:master Nov 9, 2022
RyanofWoods added a commit to RyanofWoods/solidus-solidus_paypal_braintree that referenced this pull request Nov 18, 2022
This pulls the fix from upstream (solidus_dev_support).
- solidusio/solidus_dev_support#192
RyanofWoods added a commit to RyanofWoods/solidus-solidus_paypal_braintree that referenced this pull request Dec 6, 2022
This pulls the fix from upstream (solidus_dev_support).
- solidusio/solidus_dev_support#192
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Describes or fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants