-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 generator path for namespaced models #2495
Conversation
0ccb166
to
3cbb02e
Compare
@nickcharlton I was hoping CI would kick in and run the spec I added as I don't have a compatible local setup but I guess you need to trigger that manually? Edit: added a |
8711057
to
fd31ef8
Compare
@sdwolfz, it's because the |
I think you would be open to bitcoin-mining attacks for instance if you open the build to any user |
0a1cfd0
to
ae1a75d
Compare
@dorianmarie, the biggest concern — as outlined in the blog post mentioned on the link I posted above — is really secrets exfiltration. If you were running self-hosted runners (especially so if it were reused VMs) you could likely break out of the running environment and walk up the directory too, but that's perhaps more theoretical. There are better ways to catch/dissuade mining attacks. @sdwolfz, I rebased your PR once I figured out the GHA event changes, and you should be able to push and have CI run now. Could you take a look at the failures when you get a chance? |
ae1a75d
to
0b32d59
Compare
@nickcharlton Good news is that the failures are purely from My solution is to not load the file but instead test it's contents as a string. I'm only interested in the namespace being present there anyway so it should be fine just checking the top 2 lines in this case, but what do you think? The other way would be refactoring the template so we have explicitly nested modules, meaning: module Admin
class Foo::BarsController < Admin::ApplicationController becomes module Admin
module Foo
class BarsController < Admin::ApplicationController But this is quite a big change just for the sake of a test. Not against it in principle but it's quite disruptive. |
0b32d59
to
c7d2f59
Compare
c7d2f59
to
51c0311
Compare
Ah, hah, good stuff. I think breaking out the modules would be a good idea, but probably not in this PR. If you'd be up to it, could you do that in another one? I just approved the GHA run (it shouldn't have taken me this long, but apparently 3 weeks passed!). |
51c0311
to
6b53361
Compare
Rebased with main, bundler audit failure was because of nokogiri, unrelated to changes.
Yes but I can't commit to an ETA.
I'v done worse... |
Yeah, I wouldn't expect you to commit to anything, just get to it when you can! I happened to merge in #2502 the other day, which is also generator related. Does this help at all with your testing? |
I've never generated views before so I never encountered that. Will fix linting soon. |
6b53361
to
bd01ff7
Compare
Tests are failing because of #2523 which is unrelated. |
Running a generator with a model that is namespaced, for example `Foo::Bar` puts the generated files in: - `app/controllers/admin/bars_controller.rb` - `app/dashboards/bar_dashboard.rb` The correct location is: - `app/controllers/admin/foo/bars_controller.rb` - `app/dashboards/foo/bar_dashboard.rb` This change ensures files are generated in the proper location. Without it the directory needs to be created and files need to be moved manually.
bd01ff7
to
c795236
Compare
I did a quick rebase, as we'd since solved #2523 and I'll merge it now. Thanks! |
Running a generator with a model that is namespaced, for example
Foo::Bar
puts the generated files in:app/controllers/admin/bars_controller.rb
app/dashboards/bar_dashboard.rb
The correct location is:
app/controllers/admin/foo/bars_controller.rb
app/dashboards/foo/bar_dashboard.rb
This change ensures files are generated in the proper location. Without it the directory needs to be created and files need to be moved manually.
I've also added a
docker-compose.yml
file for easy running of specs on local.