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 generator path for namespaced models #2495

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sdwolfz
Copy link

@sdwolfz sdwolfz commented Jan 25, 2024

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.

@sdwolfz sdwolfz force-pushed the generation-path branch 2 times, most recently from 0ccb166 to 3cbb02e Compare January 25, 2024 22:32
@sdwolfz
Copy link
Author

sdwolfz commented Jan 25, 2024

@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 sdwolfz force-pushed the generation-path branch 3 times, most recently from 8711057 to fd31ef8 Compare January 26, 2024 08:16
@sdwolfz sdwolfz marked this pull request as ready for review January 26, 2024 08:18
docker-compose.yml Outdated Show resolved Hide resolved
@nickcharlton
Copy link
Member

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

@dorianmarie
Copy link
Contributor

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

@nickcharlton
Copy link
Member

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

@sdwolfz
Copy link
Author

sdwolfz commented Jan 26, 2024

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

@nickcharlton
Copy link
Member

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

@sdwolfz
Copy link
Author

sdwolfz commented Feb 15, 2024

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

@nickcharlton
Copy link
Member

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?

@sdwolfz
Copy link
Author

sdwolfz commented Feb 15, 2024

I've never generated views before so I never encountered that.

Will fix linting soon.

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.
@sdwolfz
Copy link
Author

sdwolfz commented Mar 28, 2024

Tests are failing because of #2523 which is unrelated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants