feat: configuration management #158

mumer92 · 2023-11-13T08:27:00Z

Project Configuration Management

This PR introduces a way to integrate and manage config files for iOS project.

Build Phase Script Integration: Adds a script to Build Phase of Xcode. It calls Xcode build phase run script, which takes care of virtual environment and installing dependencies and calls a python script process_config.py with $CONFIGURATION and scheme_mappings argument.
Python Script for Configuration: Utilizes process_config.py for:
- Adding essential keys to Info.plist (e.g., Facebook, Microsoft keys).
- CreatingGoogleServices.plist with Firebase keys.
- Generating config.plist from ios.yaml and shared.yaml.

Inside Config.swift parsing and populating relevant keys and classes are done, e.g. AgreementConfig.swift and FirebaseConfig.swift

Get started:
Edit a config_settings.yaml in the default_config folder.
It should contain data as follows

config_directory: '{path_to_confiig_folder}'
config_mapping:
  prod: 'prod'
  stage: 'stage'
  dev: 'dev'
# These mappings are configurable, e.g. dev: 'prod_test'

config_directory provides path of config directory
config_mappings provides provides mappings that can be utilized to map Xcode build scheme map to a defined folder within config directory and it will be referenced.

Config files are separated out in 2 files

ios.yaml and shared.yaml

both configs should be placed under a folder which will be referenced inside config_settings.yaml inside of the key config_mappings:{config_directory_name}

inside config_directory, mappings.yaml which contains yaml files that will be used to read available yaml files and processed inside script.

it is structure will be as follows:

ios:
    files:
      - {file_one.yaml}
      - {file_two.yaml}

ios.yaml will contain config data specific to iOS, e.g. Firebase keys, Facebook keys, etc.
shared.yaml will contain config data that is shared, e.g. API_HOST_URL, OAUTH_CLIENT_ID, TOKEN_TYPE, etc

Future Support:

To add config related to some other service, create a class, e.g. ServiceNameConfig.swift to be able to populate related fields
Create extension to Config.swift to be able to add the newly created service as a varable to main Config.
If needed, make a protocol to be reference inside the scope of ConfigProtocol so that config is available using ConfigProtocol service.

e.g.

private let key = "KEY"
extension Config {
    public var serviceNameConfig: ServiceNameConfig {
        return ServiceNameConfig(dictionary: self[key] as? [String: AnyObject] ?? [:])
    }
}

Note
This PR also updates FirebaseCrashlytics build phase
Updated build phase script extracts googleAppID from the newly generated GoogleService-Info.plist and runs the crashlytics script with the provided id.

Examples of config files:

ios.yaml

OAUTH_CLIENT_ID: ''

FIREBASE:
  ENABLED: true
  API_KEY: "testApiKey"
  BUNDLE_ID: "testBundleID"
  CLIENT_ID: "testClientID"
  DATABASE_URL: "https://test.database.url"
  GCM_SENDER_ID: "testGCMSenderID"
  GOOGLE_APP_ID: "testGoogleAppID"
  PROJECT_ID: "testProjectID"
  REVERSED_CLIENT_ID: "testReversedClientID"
  STORAGE_BUCKET: "testStorageBucket"
  ANALYTICS_SOURCE: "firebase"
  CLOUD_MESSAGING_ENABLED: true

MICROSOFT:
  ENABLED: true
  APP_ID: "microsoftAppID"

shared.yaml

API_HOST_URL: "https://www.example.com"
FEEDBACK_EMAIL_ADDRESS: "[email protected]"
TOKEN_TYPE: "JWT"

AGREEMENT_URLS:
  PRIVACY_POLICY_URL: "https://www.example.com/privacy"
  TOS_URL: "https://www.example.com/tos"

# Features
WHATS_NEW_ENABLED: false

default_config directory is added to project to get an idea on how to write config yaml files.

OpenEdX.xcodeproj/project.pbxproj

OpenEdX/Environment.swift

Core/Core/Configuration/Combine/Config/FirebaseConfig.swift

…shlytics google app id, remove process_config.sh in favour of build script

…e scheme and a default config

mumer92 · 2023-11-14T11:48:33Z

@rnr @volodymyr-chekyrta @saeedbashir I have updated the implementation to use Xcode schemes that are currently being used. i have updated the description with these changes.

touchapp · 2023-11-15T15:21:05Z

@volodymyr-chekyrta would like your opinion here. What @rnr is proposing is more elegant and straight forward that involves less steps. I suggest we go with that. The concerns about community raised by @saeedbashir can be addressed if we make clear documentation on how to configure. @rnr can add these changes but want to make sure we are aligned.

saeedbashir · 2023-11-16T03:10:49Z

@volodymyr-chekyrta would like your opinion here. What @rnr is proposing is more elegant and straight forward that involves less steps. I suggest we go with that. The concerns about community raised by @saeedbashir can be addressed if we make clear documentation on how to configure. @rnr can add these changes but want to make sure we are aligned.

I and @rnr had a discussion on it, and we agreed on a structure. Today we'll be making the changes and then this PR will be ready for final pass.

fix: fix documentation link fix: fix documentation link fix: remove type fix: remove type fix: cleanup

mumer92 · 2023-11-20T12:08:14Z

@rnr @volodymyr-chekyrta i have made the changes that we agreed on, the PR is ready for review.

rnr · 2023-11-20T13:02:30Z

Core/Core/Configuration/Combine/Config/Config.swift

+
+extension Config: ConfigProtocol {
+    public var baseURL: URL {
+        return URL(string: string(for: ConfigKeys.baseURL.rawValue)!)!


Hello @mumer92
Could we avoid force unwrapping here?
Thank you

I understand baseURL is important part and we need to have this
maybe it can be something like

public var baseURL: URL { if let string = string(for: ConfigKeys.baseURL.rawValue), let url = URL(string: string) { return url } fatalError("Can't find baseURL in Config!") }

i used convention that was being followed in project, i think we can use guard let here and return URL(string "") instead of doing fataError, what are your thoughts?

i am not in favour of fataError(), i think app should handle issue gracefully.

@mumer92 code we have now (with force unwrapping) will just crash if baseURL is not present in config.
URL(string: "") also returns nil and will crash with force unwrapping
I understand app can't work without baseURL - we need to try to show what is the problem exactly (fatalError show a message at least). It could be some Log etc. Just provide issue description.
I'm open to any options without force unwrapping.
What do you think?

URL(string: "")! produces crash
why this Crash is better than "Crash with message"? )

alright i will add fatalError("Can't find baseURL in Config!"), in the mean time you can continue with other changes for the review.

all other looks good for me. good work @mumer92 !

@rnr i have updated it. please have a look.

@mumer92 approved
thank you

volodymyr-chekyrta · 2023-11-21T08:14:40Z

Let's move config files from the Combine folder, probably it's been placed here by mistake.

Core/Core/Configuration/Combine/Config/ConfigTests.swift

volodymyr-chekyrta · 2023-11-21T08:17:50Z

We should place the Combine framework files back in it's folder

OpenEdX/AppDelegate.swift

Core/Core/Configuration/Combine/Config/Config.swift

volodymyr-chekyrta · 2023-11-21T08:44:24Z

I played around with configurations a bit and it works amazing 🚀🚀🚀

mumer92 · 2023-11-21T08:49:59Z

@volodymyr-chekyrta i have addressed the feedback.

volodymyr-chekyrta · 2023-11-21T08:59:47Z

Green light from me 🟢 🚀
I see we are still waiting for approval from @saeedbashir
@saeedbashir, Could you please provide a review/approval?

feat: add config to process config

af58789

mumer92 requested review from volodymyr-chekyrta and saeedbashir November 13, 2023 10:27

rnr reviewed Nov 13, 2023

View reviewed changes

OpenEdX.xcodeproj/project.pbxproj Show resolved Hide resolved

rnr reviewed Nov 13, 2023

View reviewed changes

OpenEdX/Environment.swift Outdated Show resolved Hide resolved

rnr reviewed Nov 13, 2023

View reviewed changes

Core/Core/Configuration/Combine/Config/FirebaseConfig.swift Outdated Show resolved Hide resolved

chore: handle configuration provided as argument, handle firebase cra…

dc2de0b

…shlytics google app id, remove process_config.sh in favour of build script

mumer92 marked this pull request as draft November 14, 2023 08:00

chore: update implementation to provide config mappings based on Xcod…

bdfe73a

…e scheme and a default config

mumer92 marked this pull request as ready for review November 14, 2023 11:41

chore: update config test cases

7b09d04

chore: make pass dictionary to features config

7b07d05

mumer92 added 2 commits November 17, 2023 15:16

chore: update process_config.py and releated files

9e8ea21

chore: add documentation

379e78e

fix: fix documentation link fix: fix documentation link fix: remove type fix: remove type fix: cleanup

mumer92 force-pushed the umer/config branch from 21d488e to 379e78e Compare November 17, 2023 10:57

mumer92 added 2 commits November 20, 2023 11:41

fix: add default config_settings.yaml

93c61c5

chore: address feedback

dbb2155

mumer92 force-pushed the umer/config branch 2 times, most recently from b57e313 to dbb2155 Compare November 20, 2023 11:40

chore: update config initialization

39b9eca

mumer92 requested a review from rnr November 20, 2023 12:07

rnr reviewed Nov 20, 2023

View reviewed changes

chore: address feedback

b529307

mumer92 force-pushed the umer/config branch from 75e3205 to b529307 Compare November 20, 2023 18:06

rnr previously approved these changes Nov 20, 2023

View reviewed changes

volodymyr-chekyrta reviewed Nov 21, 2023

View reviewed changes

Core/Core/Configuration/Combine/Config/ConfigTests.swift Outdated Show resolved Hide resolved

volodymyr-chekyrta reviewed Nov 21, 2023

View reviewed changes

OpenEdX/AppDelegate.swift Outdated Show resolved Hide resolved

volodymyr-chekyrta reviewed Nov 21, 2023

View reviewed changes

Core/Core/Configuration/Combine/Config/Config.swift Outdated Show resolved Hide resolved

volodymyr-chekyrta reviewed Nov 21, 2023

View reviewed changes

Core/Core/Configuration/Combine/Config/Config.swift Outdated Show resolved Hide resolved

chore: address feedback

a2dce2d

mumer92 dismissed rnr’s stale review via a2dce2d November 21, 2023 08:48

volodymyr-chekyrta approved these changes Nov 21, 2023

View reviewed changes

saeedbashir approved these changes Nov 21, 2023

View reviewed changes

volodymyr-chekyrta merged commit d8267da into openedx:develop Nov 21, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: configuration management #158

feat: configuration management #158

mumer92 commented Nov 13, 2023 •

edited

Loading

mumer92 commented Nov 14, 2023 •

edited

Loading

touchapp commented Nov 15, 2023

saeedbashir commented Nov 16, 2023

mumer92 commented Nov 20, 2023

rnr Nov 20, 2023

rnr Nov 20, 2023 •

edited

Loading

mumer92 Nov 20, 2023

mumer92 Nov 20, 2023

rnr Nov 20, 2023

rnr Nov 20, 2023 •

edited

Loading

mumer92 Nov 20, 2023

rnr Nov 20, 2023

mumer92 Nov 20, 2023

rnr Nov 20, 2023

volodymyr-chekyrta commented Nov 21, 2023

volodymyr-chekyrta commented Nov 21, 2023

volodymyr-chekyrta commented Nov 21, 2023

mumer92 commented Nov 21, 2023

volodymyr-chekyrta commented Nov 21, 2023

feat: configuration management #158

feat: configuration management #158

Conversation

mumer92 commented Nov 13, 2023 • edited Loading

mumer92 commented Nov 14, 2023 • edited Loading

touchapp commented Nov 15, 2023

saeedbashir commented Nov 16, 2023

mumer92 commented Nov 20, 2023

Choose a reason for hiding this comment

rnr Nov 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnr Nov 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

volodymyr-chekyrta commented Nov 21, 2023

volodymyr-chekyrta commented Nov 21, 2023

volodymyr-chekyrta commented Nov 21, 2023

mumer92 commented Nov 21, 2023

volodymyr-chekyrta commented Nov 21, 2023

mumer92 commented Nov 13, 2023 •

edited

Loading

mumer92 commented Nov 14, 2023 •

edited

Loading

rnr Nov 20, 2023 •

edited

Loading

rnr Nov 20, 2023 •

edited

Loading