runservice: add AGOLA_RUN_COUNTER variable #473

alessandro-sorint · 2024-01-10T15:07:38Z

add AGOLA_RUN_COUNTER variable by default with run counter value to the run environment

internal/services/runservice/common/common.go

sgotti · 2024-01-11T07:28:05Z

tests/setup_test.go

+					{ type: 'run', name:'check AGOLA_RUN_COUNTER variable exists', command: '
+					    if [ "$AGOLA_RUN_COUNTER" != "%d" ]; then
+						   exit 1
+						fi


Don't do the check inside the run but in the test code. Se TestConfigContext

sgotti · 2024-01-11T07:29:35Z

tests/setup_test.go

@@ -7008,3 +7008,88 @@ func TestProjectCommitStatusRedelivery(t *testing.T) {
 		}
 	})
 }
+
+func TestAgolaRunCounterVariableIsDefined(t *testing.T) {


This test should be generalized to check all the required environment variables and the name should be something like TestRunRequiredEnvVariables.

sgotti · 2024-01-11T07:31:29Z

fix #362

#362 asked for a unique run id. The run counter is unique for the project. Is it enough?

The unique (per agola environment) run id is the run sequence.

alessandro-sorint · 2024-01-12T10:36:02Z

fix #362

#362 asked for a unique run id. The run counter is unique for the project. Is it enough?

The unique (per agola environment) run id is the run sequence.

I think we can add both counter and sequence as AGOLA_RUN_COUNTER and sequence as AGOLA_RUN_ID if you agree

sgotti · 2024-01-15T09:15:37Z

internal/services/runservice/common/common.go

@@ -127,6 +128,9 @@ func GenExecutorTaskSpecData(r *types.Run, rt *types.RunTask, rc *types.RunConfi
 	// run config Environment variables ovverride every other environment variable
 	mergeEnv(environment, rc.Environment)

+	// AGOLA_RUN_COUNTER is generated at runtime since Counter doesn't exist before creating the run


This isn't really true, counter is created before creating the run.

I mean it is not defined before call the api (it is not defined in RunCreateRequest run api types).
The counter is created in the same transaction before creating the run, but I thought it is not a good place to add variables to the run environment.

The comment could be for example AGOLA_RUN_COUNTER is generated at runtime since Counter is not defined before the save run transaction ?

I think it should just be

The AGOLA_RUN_COUNTER environment variable is not saved in the runconfig StaticEnvironment map but populated here using the run counter.

sgotti · 2024-01-15T09:20:28Z

tests/setup_test.go

+			testutil.NilError(t, err)
+
+			// update commit sha from annotations since it will change at every test
+			tt.env["AGOLA_GIT_COMMITSHA"] = run.Annotations["commit_sha"]


move it at the start of the test.

sgotti · 2024-01-15T09:22:15Z

tests/setup_test.go

+		{
+			name:      "test push with run count 1",
+			config:    fmt.Sprintf(config, 1),
+			runNumber: 1,
+			env: map[string]string{
+				"AGOLA_RUN_COUNTER":   "1",
+				"AGOLA_GIT_REF_TYPE":  "branch",
+				"AGOLA_GIT_REF":       "refs/heads/master",
+				"AGOLA_GIT_BRANCH":    "master",
+				"AGOLA_GIT_TAG":       "",
+				"AGOLA_GIT_COMMITSHA": "",
+			},
+		},
+		{
+			name:      "test push with run count 2",
+			config:    fmt.Sprintf(config, 2),
+			runNumber: 2,
+			env: map[string]string{
+				"AGOLA_RUN_COUNTER":   "2",
+				"AGOLA_GIT_REF_TYPE":  "branch",
+				"AGOLA_GIT_REF":       "refs/heads/master",
+				"AGOLA_GIT_BRANCH":    "master",
+				"AGOLA_GIT_TAG":       "",
+				"AGOLA_GIT_COMMITSHA": "",
+			},
+		},
+		{
+			name:      "test push with run count 3",
+			config:    fmt.Sprintf(config, 3),
+			runNumber: 3,
+			env: map[string]string{
+				"AGOLA_RUN_COUNTER":   "3",
+				"AGOLA_GIT_REF_TYPE":  "branch",
+				"AGOLA_GIT_REF":       "refs/heads/master",
+				"AGOLA_GIT_BRANCH":    "master",
+				"AGOLA_GIT_TAG":       "",
+				"AGOLA_GIT_COMMITSHA": "",
+			},
+		},
+		{
+			name:      "test push with run count 4",
+			config:    fmt.Sprintf(config, 4),
+			runNumber: 4,
+			env: map[string]string{
+				"AGOLA_RUN_COUNTER":   "4",
+				"AGOLA_GIT_REF_TYPE":  "branch",
+				"AGOLA_GIT_REF":       "refs/heads/master",
+				"AGOLA_GIT_BRANCH":    "master",
+				"AGOLA_GIT_TAG":       "",
+				"AGOLA_GIT_COMMITSHA": "",
+			},
+		},
+		{
+			name:      "test push with run count 5",
+			config:    fmt.Sprintf(config, 5),
+			runNumber: 5,
+			env: map[string]string{
+				"AGOLA_RUN_COUNTER":   "5",
+				"AGOLA_GIT_REF_TYPE":  "branch",
+				"AGOLA_GIT_REF":       "refs/heads/master",
+				"AGOLA_GIT_BRANCH":    "master",
+				"AGOLA_GIT_TAG":       "",
+				"AGOLA_GIT_COMMITSHA": "",
+			},
+		},


Why 5 identical tests?

sgotti · 2024-01-15T09:23:28Z

tests/setup_test.go

+				"AGOLA_GIT_REF":       "refs/heads/master",
+				"AGOLA_GIT_BRANCH":    "master",
+				"AGOLA_GIT_TAG":       "",
+				"AGOLA_GIT_COMMITSHA": "",


From this looks like AGOLA_GIT_COMMITSHA should be empty. No need to define it here if you're going to populate it later from annotations.

sgotti · 2024-01-15T10:33:34Z

internal/services/runservice/common/common.go

@@ -127,6 +128,9 @@ func GenExecutorTaskSpecData(r *types.Run, rt *types.RunTask, rc *types.RunConfi
 	// run config Environment variables ovverride every other environment variable
 	mergeEnv(environment, rc.Environment)

+	// The AGOLA_RUN_COUNTER environment variable is not saved in the runconfig StaticEnvironment map but it populated here using the run counter


I updated the comment (remove spurious 'it')

sgotti · 2024-01-15T10:34:32Z

tests/setup_test.go

+	for _, tt := range tests {
+		push(t, tt.config, giteaRepo.CloneURL, giteaToken, "commit", false)
+	}


It doesn't makes sense to push for every test. All of this should be put inside the tests loop (that could be parallelized).

tests/setup_test.go

sgotti · 2024-01-15T15:20:46Z

tests/setup_test.go

+					],
+					},
+					steps: [
+					  { type: 'run', command: 'env -u AGOLA_SSHPRIVKEY' },


is -u ... really needed?

yes because AGOLA_SSHPRIVKEY can contains value with equal characters and the ParseEnv split will fail

I'm not sure about which is the issue but there're already other tests that use don't have such issue. Anyway it should be fixed in ParseEnvs

I are investigating well, in the other tests we get the log of directruns and I see they have AGOLA_SSHPRIVKEY variable with empty value, so they work.
In my test the AGOLA_SSHPRIVKEY is a multiline variable, so I think the best way is to skip this variable with -u

sgotti · 2024-01-15T15:20:53Z

tests/setup_test.go

+				"AGOLA_GIT_REF":       "refs/heads/master",
+				"AGOLA_GIT_BRANCH":    "master",
+				"AGOLA_GIT_TAG":       "",
+				"AGOLA_GIT_COMMITSHA": "",


remove this.

sgotti · 2024-01-15T15:26:20Z

tests/setup_test.go

+		{
+			name: "test push with run count 1",
+			env: map[string]string{
+				"AGOLA_RUN_COUNTER":   "",


remove this

add AGOLA_RUN_COUNTER variable by default with run counter value to the run environment

sgotti requested changes Jan 11, 2024

View reviewed changes

alessandro-sorint force-pushed the agolaruncounter_var branch from 1d9615b to c77085f Compare January 11, 2024 10:44

alessandro-sorint changed the title ~~runservice: add AGOLA_RUN_COUNTER variable~~ runservice: add AGOLA_RUN_ID variable Jan 11, 2024

alessandro-sorint requested a review from sgotti January 12, 2024 09:32

alessandro-sorint force-pushed the agolaruncounter_var branch from c77085f to 1ffae48 Compare January 12, 2024 13:53

alessandro-sorint changed the title ~~runservice: add AGOLA_RUN_ID variable~~ runservice: add AGOLA_RUN_COUNTER variable Jan 12, 2024

alessandro-sorint force-pushed the agolaruncounter_var branch from 1ffae48 to 2af43dd Compare January 15, 2024 08:09

sgotti reviewed Jan 15, 2024

View reviewed changes

sgotti requested changes Jan 15, 2024

View reviewed changes

alessandro-sorint force-pushed the agolaruncounter_var branch from 2af43dd to d4f2907 Compare January 15, 2024 10:17

sgotti reviewed Jan 15, 2024

View reviewed changes

sgotti requested changes Jan 15, 2024

View reviewed changes

alessandro-sorint force-pushed the agolaruncounter_var branch from d4f2907 to 7c3646b Compare January 15, 2024 11:08

alessandro-sorint requested a review from sgotti January 15, 2024 13:55

sgotti requested changes Jan 15, 2024

View reviewed changes

alessandro-sorint force-pushed the agolaruncounter_var branch 2 times, most recently from 5a2aed6 to c4ae143 Compare January 16, 2024 10:01

alessandro-sorint requested a review from sgotti January 16, 2024 10:57

runservice: add AGOLA_RUN_COUNTER variable

fe0612d

add AGOLA_RUN_COUNTER variable by default with run counter value to the run environment

alessandro-sorint force-pushed the agolaruncounter_var branch from c4ae143 to fe0612d Compare January 16, 2024 12:31

sgotti approved these changes Jan 16, 2024

View reviewed changes

sgotti merged commit 9c7bc1b into agola-io:master Jan 16, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

runservice: add AGOLA_RUN_COUNTER variable #473

runservice: add AGOLA_RUN_COUNTER variable #473

alessandro-sorint commented Jan 10, 2024 •

edited

sgotti Jan 11, 2024

sgotti Jan 11, 2024 •

edited

sgotti commented Jan 11, 2024 •

edited

alessandro-sorint commented Jan 12, 2024

sgotti Jan 15, 2024

alessandro-sorint Jan 15, 2024

sgotti Jan 15, 2024 •

edited

sgotti Jan 15, 2024

sgotti Jan 15, 2024

sgotti Jan 15, 2024

sgotti Jan 15, 2024 •

edited

sgotti Jan 15, 2024

sgotti Jan 15, 2024

alessandro-sorint Jan 15, 2024

sgotti Jan 15, 2024

alessandro-sorint Jan 16, 2024

sgotti Jan 15, 2024

sgotti Jan 15, 2024

runservice: add AGOLA_RUN_COUNTER variable #473

runservice: add AGOLA_RUN_COUNTER variable #473

Conversation

alessandro-sorint commented Jan 10, 2024 • edited

Choose a reason for hiding this comment

sgotti Jan 11, 2024 • edited

Choose a reason for hiding this comment

sgotti commented Jan 11, 2024 • edited

alessandro-sorint commented Jan 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgotti Jan 15, 2024 • edited

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

sgotti Jan 15, 2024 • edited

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

alessandro-sorint commented Jan 10, 2024 •

edited

sgotti Jan 11, 2024 •

edited

sgotti commented Jan 11, 2024 •

edited

sgotti Jan 15, 2024 •

edited

sgotti Jan 15, 2024 •

edited