Skip to content

Merge all configuration #928

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

Merge all configuration #928

wants to merge 8 commits into from

Conversation

@DiamondJoseph
Copy link
Contributor

@DiamondJoseph DiamondJoseph commented Mar 17, 2025 •
edited
Loading

Parked while I return to DLS work.

  • Authenticator, AccessPolicy, Tree deserialization modified to behave like a discriminated union
  • Impls of AccessPolicy directly extend it for type checking purposes
  • Authentication top level of config merged into Settings object
  • Tree -> Trees in Settings object* [not sure about this- presumably there is a MapAdapter type that can hold a list of MapAdapters and be a root Tree instead?]
  • Authenticator.providers + Authenticator -> Authenticators in Settings object (until can reduce down to 1)
  • AccessPolicy on root of Settings documented* [not sure about this- re: Tree above, if root Tree object can put access policy on that]
  • AccessPolicy for individual trees moved out of MapAdapter object* [not sure about this- do we need on MapAdapter? Or just pass in the Tree + AccessPolicy + Path into functions that check access]

Checklist

  • Add a Changelog entry
  • Add the ticket number which this PR closes to the comment section

@DiamondJoseph DiamondJoseph force-pushed the no_authentication_config branch from 1460e56 to e1da792 Compare March 27, 2025 16:18
@DiamondJoseph DiamondJoseph force-pushed the no_authentication_config branch 2 times, most recently from 298f472 to 95aab6a Compare March 28, 2025 13:17
@DiamondJoseph DiamondJoseph force-pushed the no_authentication_config branch from 95aab6a to bcdbe08 Compare March 28, 2025 13:33
DiamondJoseph
DiamondJoseph commented Mar 31, 2025
View reviewed changes
docs/source/conf.py

app = build_app(MapAdapter({}), authentication={"authenticator": DummyAuthenticator()})
app = build_app(
MapAdapter({}), server_settings=Settings(authenticator=DummyAuthenticator())
Copy link
Contributor Author

@DiamondJoseph DiamondJoseph Mar 31, 2025

Choose a reason for hiding this comment

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

todo: move MapAdapter into Settings

example_configs/single_catalog_single_user.yml
tree: catalog
args:
tree:
type: catalog
Copy link
Contributor Author

@DiamondJoseph DiamondJoseph Mar 31, 2025

Choose a reason for hiding this comment

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

This needs to be fully qualified now

helm/tiled/values.yaml
tree: catalog
args:
tree:
type: catalog
Copy link
Contributor Author

@DiamondJoseph DiamondJoseph Mar 31, 2025

Choose a reason for hiding this comment

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

This needs to be fully qualified now
Find and replace type: catalog

tiled/_tests/test_compression.py
},
)
return build_app(tree, authentication={"single_user_api_key": "secret"})
return build_app(tree, server_settings=Settings(single_user_api_key="secret"))
Copy link
Contributor Author

@DiamondJoseph DiamondJoseph Mar 31, 2025

Choose a reason for hiding this comment

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

Pass tree into Settings

tiled/adapters/protocols.py


class AccessPolicy(Protocol):
class AccessPolicy(ABC):
Copy link
Contributor Author

@DiamondJoseph DiamondJoseph Mar 31, 2025

Choose a reason for hiding this comment

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

todo: Can AccessPolicy impls be made BaseModels, like Authenticators are? To ensure deserialization behaves

tiled/client/constructors.py
) from err
context = Context.from_app(
build_app_from_config(config, source_filepath=filepath),
build_app_from_config(merged.pop("direct", {}), source_filepath=filepath),
Copy link
Contributor Author

@DiamondJoseph DiamondJoseph Mar 31, 2025

Choose a reason for hiding this comment

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

Is "direct" another word like Authentication that needs to be excised?

tiled/server/settings.py
env_nested_delimiter="_",
)

def check_scalable(self):
Copy link
Contributor Author

@DiamondJoseph DiamondJoseph Mar 31, 2025

Choose a reason for hiding this comment

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

todo: Make this a validator for the class- currently only distinguishes the case where secret_keys has been explicitly set as an empty list or database uri has been explicitly set to blank, we need to check that those keys have been explicitly set at all.

@danielballan
Copy link
Member

danielballan commented Apr 10, 2025

We anticipate that access_control become a server-wide singleton, like authentication, instead of a setting applied separately to each tree. #951

@DiamondJoseph DiamondJoseph mentioned this pull request Jul 30, 2025
Merged
2 tasks
tpoliaw added a commit that referenced this pull request Jul 30, 2025
@tpoliaw
WIP extracting the authenticator model changes from #928
7d6c686
@DiamondJoseph DiamondJoseph mentioned this pull request Jul 31, 2025
Open
tpoliaw added a commit that referenced this pull request Aug 5, 2025
@tpoliaw
WIP extracting the authenticator model changes from #928
71a911a
@tpoliaw tpoliaw mentioned this pull request Aug 8, 2025
Merged
tpoliaw added a commit that referenced this pull request Aug 11, 2025
@tpoliaw
WIP extracting the authenticator model changes from #928
676086c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@DiamondJoseph @danielballan