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

RFC: Configurable access groups #726

Closed

Conversation

bpedersen2
Copy link
Contributor

Introduce easier overridabel compile time configuration and use it for a configurable accessgroups service.

Motivation

Configuring the access groups service needed a custom service provider factory. This needed to get copied during the build, making it hard to validate for all facilities.

This change introduces a new override config file, that can be used for injecting advanced configuration objects that are not environment vars( but as this is merged, allows to keep most defaults easily).

Tests included/Docs Updated?

  • Included for each change/fix?
  • [ needs manual test for graphql ] Passing? (Merge will not be approved unless this is checked)
  • [ no ] Docs updated?
  • New packages used/requires npm install?
  • Toggle added for new features?

@bpedersen2 bpedersen2 added the meeting Discuss in meeting before merge label Sep 11, 2023
@bpedersen2 bpedersen2 force-pushed the configurable-access-groups branch from 4116a04 to 4039b5c Compare September 12, 2023 07:38
As not all config settings can be made as environment variables,
provide a override mechanism to allow (build-time) configuration
adjustments.

An example for the  graphql access groups provider will be in the next
commit.

Change-Id: I8dc82ca4f0ac0a1b60fa47eadb147c228a77b841
@bpedersen2 bpedersen2 force-pushed the configurable-access-groups branch 3 times, most recently from 040069e to 0f44aca Compare November 23, 2023 16:49
Instead of requiring an explict service provider for each facility,
use extended configurations.

Basic enabling/disabling is implemented in the standard config via
environment vars, GraphQL needs extendend configuration via
localconfiguration.

Change-Id: I2ed630bac8f1f66d4f754e5b95d6b232ec63cf3d
Change-Id: I832b6d9e2680aa8423441924de59c4157b50c8e6
@bpedersen2
Copy link
Contributor Author

Needs local testing at ESS ( as I don't have a graphql endpoint available).

Set

ACCESS_GROUPS_GRAPHQL_ENABLED=true
ACCESS_GROUP_SERVICE_HANDLER=../../config/graphqlHandler

@bpedersen2 bpedersen2 force-pushed the configurable-access-groups branch from 0f44aca to 0ff8636 Compare November 23, 2023 16:53
@bpedersen2 bpedersen2 marked this pull request as ready for review November 23, 2023 17:00
Copy link
Member

@nitrosx nitrosx left a comment

Choose a reason for hiding this comment

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

Please see my comment on localconfiguration

src/config/localconfiguration.ts Outdated Show resolved Hide resolved
@nitrosx
Copy link
Member

nitrosx commented Dec 5, 2023

@bpedersen2 can you let me know how I can test this feature before I merge it? Thanks

@nitrosx
Copy link
Member

nitrosx commented Feb 5, 2024

It has been merged in PR #1041.
Most likely it has become obsolete and can be closed.
I will check with @Junjiequan and @bpedersen2 first

@bpedersen2
Copy link
Contributor Author

Closing as it is now part of #1041

@bpedersen2 bpedersen2 closed this Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meeting Discuss in meeting before merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants