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

refactor: replace schema struct with interface to enable flexible implementation #5426

Merged
merged 3 commits into from
Jan 16, 2025

Conversation

shekhar-rudder
Copy link
Member

@shekhar-rudder shekhar-rudder commented Jan 13, 2025

Description

Refactoring

  • Replaced the Schema struct with the SchemaHandler interface, ensuring that clients depend on the interface rather than the concrete implementation. This change will enable switching to a different schema implementation via a feature flag.
  • Schema -> schema

Linear Ticket

resolves WAR-150

Security

  • The code changed/added as part of this pull request won't create any security issues with how the software is being used.

warehouse/schema/schema.go Outdated Show resolved Hide resolved
conf: conf,
db: sqlmiddleware.New(pgResource.DB),
}
rs := redshift.New(config.New(), logger.NOP, stats.NOP)
Copy link
Member

@achettyiitr achettyiitr Jan 14, 2025

Choose a reason for hiding this comment

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

Can we use a manager in here, instead of directly accessing redshift package?

Suggested change
rs := redshift.New(config.New(), logger.NOP, stats.NOP)
whManager, err := manager.New(warehouseutils.POSTGRES, conf, logger.NOP, statsStore)
require.NoError(t, err)

Copy link
Member Author

Choose a reason for hiding this comment

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

Replaced redshift with manager. I had used redshift because that is how other tests were written

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we would need to fix other tests as well.

Copy link

codecov bot commented Jan 15, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.78%. Comparing base (b282c33) to head (e4a1e9e).
Report is 11 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5426      +/-   ##
==========================================
- Coverage   74.81%   74.78%   -0.03%     
==========================================
  Files         440      440              
  Lines       61639    61668      +29     
==========================================
+ Hits        46115    46120       +5     
- Misses      12987    13006      +19     
- Partials     2537     2542       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@shekhar-rudder shekhar-rudder merged commit cd4a7e1 into master Jan 16, 2025
60 checks passed
@shekhar-rudder shekhar-rudder deleted the var-150-schema-interface-refactoring branch January 16, 2025 08:24
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.

4 participants