administrate

administrate copied to clipboard

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.

@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 docker-compose.yml file for easy running of specs on local.

@sdwolfz, it's because the push event doesn't support running on forks where the person who's opened the PR doesn't have write access. Thanks for pointing it out, I'd forgotten about that when I sorted out GitHub Actions a few weeks back and will need to try some things out.

I think you would be open to bitcoin-mining attacks for instance if you open the build to any user

@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?

@nickcharlton Good news is that the failures are purely from load shenanigans related to non explicitly nested modules and nothing functional, so it only impacts the tests. Bad news is I can't make it work using the same patterns of tests you have.

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.

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!).

Rebased with main, bundler audit failure was because of nokogiri, unrelated to changes.

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?

Yes but I can't commit to an ETA.

it shouldn't have taken me this long, but apparently 3 weeks passed!

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.

Tests are failing because of https://github.com/thoughtbot/administrate/issues/2523 which is unrelated.

I did a quick rebase, as we'd since solved #2523 and I'll merge it now. Thanks!