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

Skip ActionCable for dummy apps #5420

Merged
merged 1 commit into from
Oct 6, 2023

Conversation

mamhoff
Copy link
Contributor

@mamhoff mamhoff commented Oct 5, 2023

Summary

We don't really use ActionCable for anything as far as I know. Having it makes installing Turbo/Stimulus in extensions harder.

The installer for Turbo will look for a Gemfile in Rails.root, which is not there in case of the dummy app, and fail. If, however, config/cable.yml can't be found, the Turbo installer succeeds with a message that Turbo is installed, just without the broadcasting feature (which is alright for most apps I believe).

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

The following are not always needed:

  • πŸ“– I have updated the README to account for my changes.
  • πŸ“‘ I have documented new code with YARD.
  • πŸ›£οΈ I have opened a PR to update the guides.
  • βœ… I have added automated tests to cover my changes.
  • πŸ“Έ I have attached screenshots to demo visual changes.

@mamhoff mamhoff requested a review from a team as a code owner October 5, 2023 16:07
@github-actions github-actions bot added the changelog:solidus_core Changes to the solidus_core gem label Oct 5, 2023
Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Looks good to me. The installer spec is failing. Mind to take a look?

@mamhoff
Copy link
Contributor Author

mamhoff commented Oct 5, 2023

Rails 7.1 came out.

@tvdeyen
Copy link
Member

tvdeyen commented Oct 5, 2023

Rails 7.1 came out.

True dat. Not sure why the installer spec fails if this happens πŸ€”

@kennyadsl
Copy link
Member

I think it's because the installer specs simulate the install process, which uses Rails at the latest available version. We need to investigate, cc @elia @rainerdema @stefano-sarioli

@elia elia self-assigned this Oct 5, 2023
@elia
Copy link
Member

elia commented Oct 5, 2023

I think it's because the installer specs simulate the install process

That's correct, I'm working on fixing the CircleCI script, meanwhile I think we can merge this, knowing the failure is a false negative πŸ‘

@elia
Copy link
Member

elia commented Oct 6, 2023

#5421 has been merged, rebasing should fix the CI

We don't really use ActionCable for anything as far as I know. Having it
makes installing Turbo/Stimulus in extensions harder.

The installer for Turbo will look for a Gemfile in `Rails.root`, which is not
there in case of the dummy app, and fail. If, however, `config/cable.yml` can't be
found, the Turbo installer succeeds with a message that Turbo is
installed, just without the broadcasting feature (which is alright for
most apps I believe).
@mamhoff mamhoff force-pushed the skip-action-cable-in-dummy-apps branch from e83bb5a to 3f8ad82 Compare October 6, 2023 09:20
@codecov
Copy link

codecov bot commented Oct 6, 2023

Codecov Report

Merging #5420 (3f8ad82) into main (2abc7d3) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #5420      +/-   ##
==========================================
- Coverage   88.86%   88.85%   -0.01%     
==========================================
  Files         607      607              
  Lines       14747    14748       +1     
==========================================
  Hits        13105    13105              
- Misses       1642     1643       +1     
Files Coverage Ξ”
core/lib/generators/spree/dummy/dummy_generator.rb 0.00% <0.00%> (ΓΈ)

πŸ“£ We’re building smart automated test selection to slash your CI/CD build times. Learn more

@elia elia merged commit bd404fe into solidusio:main Oct 6, 2023
9 of 11 checks passed
@elia elia changed the title Skip ActionCable for Dummy Apps Skip ActionCable for dummy apps Dec 21, 2023
@mamhoff
Copy link
Contributor Author

mamhoff commented Jan 25, 2024

Yes, this does need to be backported to solidus 3.4 - 4.2. I can't add the backport labels due to lack of permission. Can anyone?

@kennyadsl kennyadsl added backport-v3.4 Backport this pull-request to v3.4 backport-v4.0 Backport this pull-request to v4.0 backport-v4.1 Backport this pull-request to v4.1 backport-v4.2 Backport this pull-request to v4.2 labels Jan 25, 2024
Copy link

πŸ’š All backports created successfully

Status Branch Result
βœ… v3.4
βœ… v4.0
βœ… v4.1
βœ… v4.2

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details

Copy link

πŸ’š All backports created successfully

Status Branch Result
βœ… v3.4
βœ… v4.0
βœ… v4.1
βœ… v4.2

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details

Copy link

πŸ’š All backports created successfully

Status Branch Result
βœ… v3.4
βœ… v4.0
βœ… v4.1
βœ… v4.2

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details

Copy link

πŸ’š All backports created successfully

Status Branch Result
βœ… v3.4
βœ… v4.0
βœ… v4.1
βœ… v4.2

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details

@mamhoff mamhoff deleted the skip-action-cable-in-dummy-apps branch January 30, 2024 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-v3.4 Backport this pull-request to v3.4 backport-v4.0 Backport this pull-request to v4.0 backport-v4.1 Backport this pull-request to v4.1 backport-v4.2 Backport this pull-request to v4.2 changelog:solidus_core Changes to the solidus_core gem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants