Skip to content

Conversation

samfreund
Copy link
Member

Description

We don't need this anymore for backwards compat, so we're getting rid of it! Less code, less clutter.

Meta

Merge checklist:

  • Pull Request title is short, imperative summary of proposed changes
  • The description documents the what and why
  • If this PR changes behavior or adds a feature, user documentation is updated
  • If this PR touches photon-serde, all messages have been regenerated and hashes have not changed unexpectedly
  • If this PR touches configuration, this is backwards compatible with settings back to v2024.3.1
  • If this PR touches pipeline settings or anything related to data exchange, the frontend typing is updated
  • If this PR addresses a bug, a regression test for it is added

@samfreund samfreund requested a review from a team as a code owner August 15, 2025 20:34
@github-actions github-actions bot added the backend Things relating to photon-core and photon-server label Aug 15, 2025
@Gold856
Copy link
Collaborator

Gold856 commented Aug 16, 2025

I think you'll need to remove some of older network settings tests. May as well just go through and clean up all the old configs in test-resources as well.

@samfreund
Copy link
Member Author

I think you'll need to remove some of older network settings tests. May as well just go through and clean up all the old configs in test-resources as well.

Likely, I just kinda ripped and touched the tests a little bit. I'll go through more thoroughly later when I have the chance.

@samfreund samfreund force-pushed the rip-legacy branch 2 times, most recently from 708bafa to d213616 Compare August 17, 2025 05:36
@Gold856
Copy link
Collaborator

Gold856 commented Aug 17, 2025

Are we sure we don’t want to change the database schema or anything? I think Matt was interested removing the separate JSON columns in favor of unifying all of them into one.

@samfreund
Copy link
Member Author

Are we sure we don’t want to change the database schema or anything? I think Matt was interested removing the separate JSON columns in favor of unifying all of them into one.

Like unifying all the various settings and config files?

@crschardt
Copy link
Contributor

Are we sure we don’t want to change the database schema or anything? I think Matt was interested removing the separate JSON columns in favor of unifying all of them into one.

Let’s discuss that in an issue and come up with a plan for a different structure.

Copy link
Member Author

Choose a reason for hiding this comment

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

@mcm001 What exactly does this test? I'm trying to figure out if I can just dump it since it covers the legacy config, and if dumping it means we need tests for the sql config too.

Copy link
Contributor

Choose a reason for hiding this comment

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

You should keep the Jackson tests iff the upgrade/migration they handle was added in 2025. Otherwise they don't need to stick around.

@Gold856 Gold856 linked an issue Aug 20, 2025 that may be closed by this pull request
@Gold856
Copy link
Collaborator

Gold856 commented Aug 20, 2025

Do we want to resolve #1488 here as well?

@mcm001
Copy link
Contributor

mcm001 commented Aug 31, 2025

I would vote to keep the 1488 diff separate, just to make keeping track of commits once this is squashed more clear

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Things relating to photon-core and photon-server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants