Add EnvironmentVariable struct as a RunAction's Arguments' parameter #3320

setoelkahfi · 2021-08-22T08:28:00Z

Resolves #3257
Request for comments document (if applies):

Short description 📝

Add the ability to enable or disable environment variables that are passed when running a scheme.

Checklist ✅

The code architecture and patterns are consistent with the rest of the codebase.
The changes have been tested following the documented guidelines.
The CHANGELOG.md has been updated to reflect the changes. In case of a breaking change, it's been flagged as such.
In case the PR introduces changes that affect users, the documentation has been updated.

kwridan

Nice work @setoelkahfi! thanks for submitting this.

A few small tidbits are still needed:

The documentation can be updated to reflect the changes the environment type - those can be found here https://github.com/tuist/tuist/blob/main/projects/docs/docs/manifests/project.md
We can possibly extend one of the fixtures (e.g. https://github.com/tuist/tuist/blob/main/projects/tuist/fixtures/ios_app_with_custom_scheme/App/Project.swift to leverage the new environment properties)
In the changelog we can mark this change as Breaking to help clarify this is a breaking change in 2.0

kwridan · 2021-08-24T07:31:18Z

Sources/TuistCache/ContentHashing/TargetContentHasher.swift

@@ -85,7 +85,6 @@ public final class TargetContentHasher: TargetContentHashing {
        let coreDataModelHash = try coreDataModelsContentHasher.hash(coreDataModels: graphTarget.target.coreDataModels)
        let targetActionsHash = try targetActionsContentHasher.hash(targetActions: graphTarget.target.actions)
        let dependenciesHash = try dependenciesContentHasher.hash(graphTarget: graphTarget, hashedTargets: &hashedTargets)
-        let environmentHash = try contentHasher.hash(graphTarget.target.environment)
        var stringsToHash = [


is this intended? To maintain the previous behaviour we'd need to continue hashing the environment values :/

No.
I was just trying to make it build successfully. I guess try contentHasher.hash(graphTarget.target.environmentVariables.map { $0.value }) should be fine?

Let's make EnvironmentVariable conform to hashable so it is synthesized by the compiler and we do not need the map here :)

Let's make EnvironmentVariable conform to hashable so it is synthesized by the compiler and we do not need the map here :)

Oh yeah, I missed that one.

Hey @adellibovi,
I don't think I understand this. So, I got the graphTarget.target.environmentVariables.hashValue by conforming the EnvironmentVariable to the Hashable protocol. Where should I put it?

Oops, you are right @setoelkahfi, that is not needed, got confused by Swift hash vs our Content hash.
Though it seems to me we would need to hash all three properties instead of just name, as changing one of those should invalidate the cache, @kwridan can you confirm?

@adellibovi What I can think of right now is to convert the environment variable array into a single string and then hash it with the content hasher.

kwridan · 2021-08-24T07:32:41Z

Sources/TuistGenerator/Generator/SchemeDescriptorsGenerator.swift

-            XCScheme.EnvironmentVariable(variable: key, value: value, enabled: true)
+    private func environmentVariables(_ environments: [EnvironmentVariable]) -> [XCScheme.EnvironmentVariable] {
+        environments.map { environment in
+            XCScheme.EnvironmentVariable(variable: environment.key, value: environment.value, enabled: environment.isEnabled)
        }.sorted { $0.variable < $1.variable }


I suspect we no longer need to sort as we'll now have a stable order as we'll persist the user defined order.

I removed calling the sorted function.

kwridan · 2021-08-24T07:40:06Z

Tests/TuistLoaderTests/Extensions/TuistTestCase+ManifestMappers.swift

+        let rawEnvironments: [EnvironmentVariable: Bool] = arguments.environmentVariables.reduce(into: [:]) { _, _ in }
+        let rawEnvironmenstManifest = manifest.environmentVariables.reduce(into: [:]) { _, _ in }


The goal here is to compare the raw values of the manifest types and the model types, this can be achieved in any number of ways. We can do something along the lines of:

XCTAssertEqual(arguments.environmentVariables.count, manifest.environmentVariables.count) zip(arguments.environmentVariables, manifest.environmentVariables).forEach { model, manifest in XCTAssertEqual(model.isEnabled, manifest.isEnabled) XCTAssertEqual(model.key, manifest.key) XCTAssertEqual(model.value, manifest.value) }

fortmarek · 2021-08-29T13:09:51Z

Could you try to rebase from main to fix the CI? 🙏

setoelkahfi · 2021-08-29T13:55:47Z

Could you try to rebase from main to fix the CI? 🙏

Will do.

setoelkahfi · 2021-08-29T20:23:22Z

It seems like it's getting worse :). I believe you mean to try to rebase my working branch onto the main branch @fortmarek.

laxmorek · 2021-08-30T06:12:19Z

Hey @setoelkahfi 👋

Could you change your target branch to main? We've decided to continue 2.0 development on main.

What is more I believe that some of CI problems will be resolved once you rebase onto main and change target branch.

I will try to help you with others once CI re-run.

setoelkahfi · 2021-08-30T06:56:01Z

Hey @setoelkahfi 👋

Could you change your target branch to main? We've decided to continue 2.0 development on main.

What is more I believe that some of CI problems will be resolved once you rebase onto main and change target branch.

I will try to help you with others once CI re-run.

Hey @laxmorek,
I changed the target branch to main.

laxmorek · 2021-08-30T07:33:56Z

I'm very sorry, we reconsider our approach and decided to keep release-2.0 branch live.

From Slack:

We consider your branch as breaking so Ive update target branch one more time. It should be release-2.0 (like you did before).

Again, sorry for being misleading.

setoelkahfi · 2021-08-30T18:03:43Z

I'm very sorry, we reconsider our approach and decided to keep release-2.0 branch live.

From Slack:

We consider your branch as breaking so Ive update target branch one more time. It should be release-2.0 (like you did before).

Again, sorry for being misleading.

No worries. Thanks for clarifying.

github-actions · 2021-10-02T02:11:11Z

This PR has been marked as stale because it's been opened for more than 30 days. If no action is taken, it'll be closed in 5 days.

kwridan reviewed Aug 24, 2021

View reviewed changes

setoelkahfi marked this pull request as ready for review August 26, 2021 21:27

danieleformichelli added the tuist-2.0 label Aug 27, 2021

setoelkahfi added 7 commits August 29, 2021 20:48

Add EnvironmentVariable struct

4e1bd8f

Add documentation for the new EnvironmentVariable type

5ac6985

Add documentation

01ac5f9

Remove calling sorted method

49c0d7e

Fix EnvironmentVariable assertion

0c5ccfd

Hash the environment variable values\

c00640e

Update changelog

ce09149

setoelkahfi force-pushed the feature/add-environment-variable-struct branch from 972fe72 to ce09149 Compare August 29, 2021 18:48

setoelkahfi changed the title ~~Add EnvironmentVariable struct~~ Add EnvironmentVariable struct as a RunAction's Arguments' parameter Aug 29, 2021

setoelkahfi changed the base branch from release-2.0 to main August 30, 2021 06:45

laxmorek changed the base branch from main to release-2.0 August 30, 2021 07:29

laxmorek mentioned this pull request Aug 30, 2021

Release 2.0 #3374

Merged

4 tasks

github-actions bot added the no-pr-activity label Oct 2, 2021

pepicrft deleted the branch tuist:release-2.0 October 2, 2021 10:23

pepicrft closed this Oct 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add EnvironmentVariable struct as a RunAction's Arguments' parameter #3320

Add EnvironmentVariable struct as a RunAction's Arguments' parameter #3320

setoelkahfi commented Aug 22, 2021 •

edited

kwridan left a comment

kwridan Aug 24, 2021

setoelkahfi Aug 26, 2021

adellibovi Aug 30, 2021

setoelkahfi Aug 31, 2021

setoelkahfi Aug 31, 2021

adellibovi Aug 31, 2021

setoelkahfi Sep 1, 2021

kwridan Aug 24, 2021

setoelkahfi Aug 26, 2021

kwridan Aug 24, 2021

setoelkahfi Aug 26, 2021

fortmarek commented Aug 29, 2021

setoelkahfi commented Aug 29, 2021

setoelkahfi commented Aug 29, 2021

laxmorek commented Aug 30, 2021 •

edited

setoelkahfi commented Aug 30, 2021 •

edited

laxmorek commented Aug 30, 2021

setoelkahfi commented Aug 30, 2021

github-actions bot commented Oct 2, 2021

		let rawEnvironments: [EnvironmentVariable: Bool] = arguments.environmentVariables.reduce(into: [:]) { _, _ in }
		let rawEnvironmenstManifest = manifest.environmentVariables.reduce(into: [:]) { _, _ in }

Add EnvironmentVariable struct as a RunAction's Arguments' parameter #3320

Add EnvironmentVariable struct as a RunAction's Arguments' parameter #3320

Conversation

setoelkahfi commented Aug 22, 2021 • edited

Short description 📝

Checklist ✅

kwridan left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fortmarek commented Aug 29, 2021

setoelkahfi commented Aug 29, 2021

setoelkahfi commented Aug 29, 2021

laxmorek commented Aug 30, 2021 • edited

setoelkahfi commented Aug 30, 2021 • edited

laxmorek commented Aug 30, 2021

setoelkahfi commented Aug 30, 2021

github-actions bot commented Oct 2, 2021

setoelkahfi commented Aug 22, 2021 •

edited

laxmorek commented Aug 30, 2021 •

edited

setoelkahfi commented Aug 30, 2021 •

edited