Skip to content

Fixes #35330 - Enable Active Storage framework - #11314

Merged
adamruzicka merged 1 commit into
theforeman:developfrom
jakduch:feature/active-storage-foundation
Oct 14, 2026
Merged

adamruzicka merged 1 commit into
theforeman:developfrom
jakduch:feature/active-storage-foundation

Conversation

@jakduch

@jakduch jakduch commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

What

Enable the Rails Active Storage engine, add its database schema, and configure isolated disk services for development, test, and production. Generic Active Storage routes stay disabled so each consumer must authorize attachment access in its own controller.

This revives the framework portion of #9339 without the PostgreSQL blob adapter. S3 configuration and plugin migrations can follow as separate, reviewable changes. The RPM runtime is being prepared in foreman-packaging#14207 and its dependency chain.

Test

  • PostgreSQL 13 migration
  • test/unit/active_storage_test.rb: isolated test service, blob write/read/purge round-trip, and disabled generic attachment routes
  • RuboCop on all changed Ruby files

Developed with assistance from OpenAI Codex (GPT-5.6 Sol High).

@jakduch

jakduch commented Oct 3, 2026

Copy link
Copy Markdown
Contributor Author

The JavaScript failures are unrelated date-boundary flakes in the existing DateTimePicker tests. The run just before UTC midnight failed the “current date with future time” case because +1 hour crossed into the next day while the test kept the old date; the run just after midnight failed the symmetric “current date with past time” case because -1 hour crossed into the previous day while keeping the new date. See the pre-midnight job and post-midnight job. This PR does not change JavaScript or DateTimePicker.

Enable Rails Active Storage with local disk services and its database schema. Keep generic blob routes disabled so consumers retain authorization control.

Co-Authored-By: OpenAI Codex (GPT-5.6 Sol High) <noreply@openai.com>
@jakduch
jakduch force-pushed the feature/active-storage-foundation branch from 3d489d4 to 371b3aa Compare October 3, 2026 13:22
@lzap
lzap requested review from ShimShtein and a balanced review from Copilot October 9, 2026 06:38
@lzap

lzap commented Oct 9, 2026

Copy link
Copy Markdown
Member

Misclicked and requested CoPilot review, sorry.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Approval recommended

The implementation is consistent with Rails 7 conventions and includes focused coverage for the enabled behavior.

0 open findings

What changed in this PR

Enables Rails Active Storage with isolated disk services while keeping generic attachment routes disabled.

Changes:

  • Enables the Active Storage engine and database schema.
  • Configures environment-specific disk storage.
  • Tests storage round-trips and route isolation.
File Description
config/​application.rb Enables Active Storage and disables generic routes.
config/​storage.yml Defines disk storage services.
config/​environments/​development.rb Selects development storage.
config/​environments/​test.rb Selects isolated test storage.
config/​environments/​production.rb Selects production-local storage.
db/​migrate/​20261003000000_create_active_storage_tables.rb Adds Active Storage tables and constraints.
test/​unit/​active_storage_test.rb Verifies configuration, persistence, cleanup, and disabled routes.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@stejskalleos

Copy link
Copy Markdown
Contributor

What's the use case here? What Foreman's features require active storage?

@adamruzicka

Copy link
Copy Markdown
Contributor

I'm pretty sure there are places (openscap comes to mind) where we had to roll our own blob storage just because we had no other option. I'd be in favor of making things as vanilla rails-y as possible and this feels like a step in that direction, even if we don't immediately require it as a dependency for something else.

@stejskalleos stejskalleos left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd be in favor of making things as vanilla rails-y as possible and this feels like a step in that direction, even if we don't immediately require it as a dependency for something else.

Agree.

The code changes LGTM, I'd like to have second ack, just for sure, @adamruzicka please

@adamruzicka adamruzicka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I let AI quickly cobble up a poc for the openscap consumer of this, seems to work well.

@adamruzicka
adamruzicka merged commit d69782d into theforeman:develop Oct 14, 2026
36 checks passed
@adamruzicka

Copy link
Copy Markdown
Contributor

Thank you @jakduch !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

5 participants