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.

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

MOB-1817 Environment staging updates #479

Merged
merged 18 commits into from
Jul 4, 2023
Merged

MOB-1817 Environment staging updates #479

merged 18 commits into from
Jul 4, 2023

Conversation

d4r1091
Copy link
Member

@d4r1091 d4r1091 commented Jun 23, 2023 •
edited
Loading

MOB-1817

Context

This PR integrates the changes done in the Core module.

Approach

The key updates here are the build configuration renaming, in order to match the requirement endpoints.

Debug -> Staging endpoints
BetaDebug -> Staging endpoints
Development_TestFlight -> Staging endpoints
Development_AppCenter -> Staging endpoints
Release -> Production endpoints

It also integrates the updates to make the autocomplete done via the staging endpoint bypassing the challenge.

Tests performed

  • Build and run the EcosiaBeta scheme => BetaDebug configuration

    • OUTCOME: Staging endpoint ✅

  • Build and run the Ecosia scheme => Debug configuration

    • OUTCOME: Staging endpoint ✅

  • Triggered AppCenter build over CircleCI EcosiaBeta scheme => Development_AppCenter configuration

    • OUTCOME: Staging endpoint ✅

  • Triggered TestFlight build over CircleCI EcosiaBeta scheme => Development_TestFlight configuration

    • OUTCOME: Staging endpoint ✅

  • Triggered TestFlight build over CircleCI Ecosia scheme => Release configuration

    • OUTCOME: Production endpoint ✅

Other

  • I could also test the autoincrement of a real release and works.
  • I may need a PR to fine-tune it. Different scope though ☝️

🚨Last commit to be reverted. Needed to pass tests only 😉

@d4r1091 d4r1091 marked this pull request as draft June 23, 2023 14:13
@d4r1091 d4r1091 force-pushed the MOB-1817_environment_staging_updates branch 3 times, most recently from af4ef68 to 3470898 Compare June 27, 2023 14:11
@d4r1091 d4r1091 requested a review from a team June 27, 2023 15:20
@d4r1091 d4r1091 marked this pull request as ready for review June 27, 2023 15:20
ecotopian
ecotopian reviewed Jun 28, 2023
View reviewed changes
Client.xcodeproj/project.pbxproj
@@ -12351,14 +12351,14 @@
};
name = Release;
};
3B43E3D91D95C48E00BBA9DB /* Beta */ = {
3B43E3D91D95C48E00BBA9DB /* Development_TestFlight */ = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's kind of counter-intuitive for me to find both words Development and TestFlight in the the name of the configuration. My brain associates the term Development with a Debug configuration, and when building for TestFlight usually a Release-like config is used.

So for clarity, the term Development must be included, so that SPM in iOS-core does infer it as non-#Production environment? Did I catch that correctly?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the comment @ecotopian . I agree it does sound counter intuitive. I've gone through the reason of choosing this approach more thoroughly in the Core PR.
I was planning to update the README as well as I do think that indeed providing additional knowledge to it would concretely help clarifying.

@d4r1091 d4r1091 force-pushed the MOB-1817_environment_staging_updates branch 2 times, most recently from 6f7802d to 925aafd Compare July 3, 2023 07:41
@d4r1091 d4r1091 requested a review from a team July 3, 2023 08:28
lucaschifino
lucaschifino reviewed Jul 3, 2023
View reviewed changes
Copy link

@lucaschifino lucaschifino left a comment

Choose a reason for hiding this comment

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

Looking good to me 🚀

Will just wait for the final review and approval after Core is properly updated.

Client/Application/AppDelegate.swift
@@ -34,9 +34,6 @@ class AppDelegate: UIResponder, UIApplicationDelegate {

appLaunchUtil = AppLaunchUtil(profile: profile)
appLaunchUtil?.setUpPreLaunchDependencies()

// Setup environment
Environment.current = AppConstants.BuildChannel == .release ? .production : .staging
Copy link

Choose a reason for hiding this comment

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

🧹 💯

README.md
#### Build configurations

The app is equipped by two custom Build Configurations for ad-hoc distribution over TestFlight and AppCenter.
The `Development_` prefix added to those two, serves the purpose of picking the correct `Core` module build configuration.
Copy link

Choose a reason for hiding this comment

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

Quick and clear 👏

If you think it is worth it, could even link to the ADR.

Copy link
Member Author

Choose a reason for hiding this comment

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

The only reason I woulnd't want to link it is due to this repo being public and people will land into the 404 Github page 😕 . I know it's widely practiced for Jira tickets for other OSS projects but when it comes to Github repo it may be somewhat unpleasant to find into a README doc 🤔

d4r1091 added 18 commits July 4, 2023 09:47
@d4r1091
Updated the browser app with the concept of urlProvider
a9b0d22
@d4r1091
Updated swift package
f89a1fd
@d4r1091
Fix search test
7be5466
@d4r1091
Forced app center deploy
8222ada
@d4r1091
Updated request params
453ea3d
@d4r1091
Updated SPM
7a73fcd
@d4r1091
Reviewed Build Configurations
86d834e
@d4r1091
Updated build configuration name
8621aa9
@d4r1091
Updated versions for Beta TF
263888b
@d4r1091
Forced TF deploy
2778957
@d4r1091
Revert "Forced TF deploy"
d0f872e 
This reverts commit 3470898.
@d4r1091
Revert "Forced app center deploy"
a760da0 
This reverts commit 1805b5a.
@d4r1091
Revert "Updated swift package"
ac97817 
This reverts commit 444f6f4.

# Conflicts:
#	Client.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
@d4r1091
Adapting changes based on latest work
9b05189
@d4r1091
Updated README
9adea3f
@d4r1091
TO REVERT: Updated SPM to pass tests
c5a08dd
@d4r1091
Revert "TO REVERT: Updated SPM to pass tests"
aed73d7 
This reverts commit 925aafd.
@d4r1091
Updated SPM
17ccaf4
@d4r1091 d4r1091 force-pushed the MOB-1817_environment_staging_updates branch from 7419170 to 17ccaf4 Compare July 4, 2023 07:48
@d4r1091 d4r1091 mentioned this pull request Jul 4, 2023
Merged
lucaschifino
lucaschifino approved these changes Jul 4, 2023
View reviewed changes
Copy link

@lucaschifino lucaschifino left a comment

Choose a reason for hiding this comment

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

LGTM 🟢

ecotopian
ecotopian approved these changes Jul 4, 2023
View reviewed changes
Copy link
Collaborator

@ecotopian ecotopian left a comment

Choose a reason for hiding this comment

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

Looking great now!

@d4r1091 d4r1091 merged commit bfe73c1 into main Jul 4, 2023
2 checks passed
@d4r1091 d4r1091 deleted the MOB-1817_environment_staging_updates branch July 4, 2023 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lucaschifino lucaschifino lucaschifino approved these changes

@ecotopian ecotopian ecotopian approved these changes

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
@d4r1091 @lucaschifino @ecotopian