Skip to content

Conversation

@eric-maynard
Copy link
Contributor

FILE has been included in the default value for SUPPORTED_CATALOG_STORAGE_TYPES since the config's inception, but we should consider removing it as users who don't read through configuring-polaris-for-production.md might be inadvertently leaving an insecure storage type enabled.

Copy link
Member

@snazy snazy left a comment

Choose a reason for hiding this comment

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

This is what #1532 already does - there's no need for a 2nd PR for the same thing.

@eric-maynard
Copy link
Contributor Author

eric-maynard commented May 12, 2025

#1532 does a lot more than this. See this comment on that PR.

dimas-b
dimas-b previously approved these changes May 13, 2025
Copy link
Contributor

@dimas-b dimas-b left a comment

Choose a reason for hiding this comment

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

This PR makes sense to me end-to-end.

I agree that it conflicts with #1532 is some areas, but at it stands this PR appears to be self-sufficient, consistent and offers net improvement for user experience (removes defaults that users should not use anyway).

I would not mind merging this PR and rebasing #1532 since the latter has a bigger scope of changes and has some comment threads that need to be resolved.

@snazy WDYT?

@eric-maynard eric-maynard marked this pull request as ready for review May 13, 2025 23:12
@eric-maynard eric-maynard requested a review from singhpk234 as a code owner May 13, 2025 23:12
Copy link
Contributor

@adnanhemani adnanhemani left a comment

Choose a reason for hiding this comment

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

Overall, agreed with this change as well. One question - I know we've modified the integration tests, have we confirmed when running them that they continue to work?

@eric-maynard
Copy link
Contributor Author

@snazy can you clarify if there is a technical reason for the -1 here?

@snazy
Copy link
Member

snazy commented May 20, 2025

Sure, #1532 has to go in first.

@snazy
Copy link
Member

snazy commented May 20, 2025

And this one duplicates work from #1532

@eric-maynard
Copy link
Contributor Author

eric-maynard commented May 20, 2025 via email

@github-project-automation github-project-automation bot moved this from PRs In Progress to Done in Basic Kanban Board May 21, 2025
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