Add yaml outputs of resources to streaming logs and add import&update steps to uptest #136

sergenyalcin · 2023-09-28T11:01:49Z

Description of your changes

This PR adds yaml outputs of resources to streaming log while waiting the Ready condition. Kuttl has a script field for supporting multiple commands.
This PR adds new two test steps to uptest: import and update

I have:

Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

Tested locally.

Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>

internal/templates/01-update.yaml.tmpl

Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>

ulucinar

Thanks @sergenyalcin, lgtm. If we have a bug in the resource updates, let's not hide that as we have discussed.

ulucinar · 2023-10-05T11:19:16Z

.github/workflows/pr-comment-trigger.yml

@@ -17,6 +17,10 @@ on:
        default: 'provider'
        required: false
        type: string
+      update-test-parameter:


nit: We may consider adding a description to the new parameter.

Let's also document that the update parameter should be a serialized JSON object and if the JSON object has nested children, then there must be only one leaf.

ulucinar · 2023-10-05T11:34:22Z

internal/templates/00-assert.yaml.tmpl

@@ -12,7 +13,7 @@ commands:
 {{- end }}
 {{- range $condition := $resource.Conditions }}
 {{- if $resource.Namespace }}
- command: ${KUBECTL} wait {{ $resource.KindGroup }}/{{ $resource.Name }} --for=condition={{ $condition }} --timeout 10s --namespace {{ $resource.Namespace }}
+- script: ${KUBECTL} get --namespace {{ $resource.Namespace }} {{ $resource.KindGroup }}/{{ $resource.Name }} -o yaml && ${KUBECTL} wait {{ $resource.KindGroup }}/{{ $resource.Name }} --for=condition={{ $condition }} --timeout 10s --namespace {{ $resource.Namespace }}


We may just dump all the claims above with the MRs:

Suggested change

- script: ${KUBECTL} get --namespace {{ $resource.Namespace }} {{ $resource.KindGroup }}/{{ $resource.Name }} -o yaml && ${KUBECTL} wait {{ $resource.KindGroup }}/{{ $resource.Name }} --for=condition={{ $condition }} --timeout 10s --namespace {{ $resource.Namespace }}

- script: ${KUBECTL} wait {{ $resource.KindGroup }}/{{ $resource.Name }} --for=condition={{ $condition }} --timeout 10s --namespace {{ $resource.Namespace }}

ulucinar · 2023-10-05T11:35:55Z

internal/templates/00-assert.yaml.tmpl

@@ -3,6 +3,7 @@ kind: TestAssert
 timeout: {{ .TestCase.Timeout }}
 commands:
 - command: ${KUBECTL} annotate managed --all upjet.upbound.io/test=true --overwrite
+- command: ${KUBECTL} get managed -o yaml


Suggested change

- command: ${KUBECTL} get managed -o yaml

- script: ${KUBECTL} get managed -o yaml || ${KUBECTL} get claim --all-namespaces -o yaml

ulucinar · 2023-10-05T11:37:08Z

internal/templates/00-assert.yaml.tmpl

@@ -3,6 +3,7 @@ kind: TestAssert
 timeout: {{ .TestCase.Timeout }}
 commands:
 - command: ${KUBECTL} annotate managed --all upjet.upbound.io/test=true --overwrite
+- command: ${KUBECTL} get managed -o yaml


nit: We had better include a header in the debug log messages like Dumping all MRs: , Dumping all claims: .

ulucinar · 2023-10-05T11:40:32Z

internal/templates/01-update.yaml.tmpl

+{{- if eq $resource.KindGroup "secret." -}}
+  {{continue}}
+{{- end -}}
+{{- if not $resource.Namespace }}


In the next iteration, we can consider adding claim update tests.

ulucinar · 2023-10-05T11:57:19Z

internal/tester.go

+	for key, value := range data {
+		newPath := currentPath + "." + key
+		switch v := value.(type) {
+		case map[string]interface{}:


We may capture in the description of the update parameter that only maps & nested objects are supported.

ulucinar · 2023-10-05T12:11:26Z

internal/tester.go

@@ -81,6 +82,15 @@ func (t *tester) prepareConfig() (*config.TestCase, []config.Resource, error) {
 			Conditions: t.options.DefaultConditions,
 		}

+		if updateParameter := os.Getenv("UPTEST_UPDATE_PARAMETER"); updateParameter != "" {


Suggested change

if updateParameter := os.Getenv("UPTEST_UPDATE_PARAMETER"); updateParameter != "" {

updateParameter := ... // get value from annotation

if updateParameter == "" {

updateParameter = os.Getenv("UPTEST_UPDATE_PARAMETER")

}

if updateParameter != "" {

...

ulucinar · 2023-10-05T12:23:22Z

internal/templates/02-import.yaml.tmpl

+- command: ${KUBECTL} --subresource=status patch {{ $resource.KindGroup }}/{{ $resource.Name }} --type=merge -p '{"status":{"conditions":[]}}'
+- script: ${KUBECTL} annotate {{ $resource.KindGroup }}/{{ $resource.Name }} uptest-old-id=$(${KUBECTL} get {{ $resource.KindGroup }}/{{ $resource.Name }} -o=jsonpath='{.status.atProvider.id}') --overwrite


nit: We have better scale the provider deployment down before updating the status and putting the annotation here.

ulucinar · 2023-10-05T12:56:34Z

internal/templates/03-assert.yaml.tmpl

+  {{continue}}
+{{- end -}}
+{{- if $resource.Namespace }}
+- script: ${KUBECTL} get --namespace {{ $resource.Namespace }} {{ $resource.KindGroup }}/{{ $resource.Name }} -o yaml --ignore-not-found && ${KUBECTL} wait {{ $resource.KindGroup }}/{{ $resource.Name }} --for=delete --timeout 10s --namespace {{ $resource.Namespace }}


We may consider moving the kubectl get outside the loop.

ulucinar · 2023-10-05T12:57:46Z

internal/templates/embed.go

+
+// assertUpdatedFileTemplate is the template for update assert file.
+//
+//go:embed 01-assert.yaml.tmpl


nit:

Suggested change

//go:embed 01-assert.yaml.tmpl

//go:embed 01-assert-update.yaml.tmpl

ulucinar · 2023-10-05T13:07:56Z

internal/templates/01-update.yaml.tmpl

+  {{continue}}
+{{- end -}}
+{{- if not $resource.Namespace }}
+- command: ${KUBECTL} patch {{ $resource.KindGroup }}/{{ $resource.Name }} --type=merge -p '{"spec":{"forProvider":{{ $resource.UpdateParameter }}}}'


We had better only patch the target manifest in the update tests.

Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>

Add yaml outputs of resources to during wait conditions

6368327

Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>

sergenyalcin requested a review from ulucinar as a code owner September 28, 2023 11:01

sergenyalcin added 2 commits September 28, 2023 14:44

Fix unit tests

b00f8c8

Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>

Add update and import steps to uptest

3b210de

Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>

sergenyalcin changed the title ~~Add yaml outputs of resources to streaming log while waiting the Ready condition~~ Add yaml outputs of resources to streaming logs and add import&update steps to uptest Oct 3, 2023

sergenyalcin self-assigned this Oct 3, 2023

ulucinar reviewed Oct 3, 2023

View reviewed changes

internal/templates/01-update.yaml.tmpl Show resolved Hide resolved

ulucinar reviewed Oct 3, 2023

View reviewed changes

internal/templates/01-update.yaml.tmpl Outdated Show resolved Hide resolved

sergenyalcin added 2 commits October 4, 2023 18:16

Fix unit tests and make optional the update step

cee19d8

Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>

Store id field instead of external name

21da281

Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>

sergenyalcin force-pushed the improvements branch from 9ce4c07 to 21da281 Compare October 5, 2023 10:07

ulucinar approved these changes Oct 5, 2023

View reviewed changes

ulucinar reviewed Oct 5, 2023

View reviewed changes

Address reviews

740e2fc

Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>

sergenyalcin force-pushed the improvements branch from 4ed2f67 to 740e2fc Compare October 5, 2023 15:00

Fix linter issue

61dd809

Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>

sergenyalcin merged commit 61e5bb8 into upbound:main Oct 5, 2023
6 checks passed

sergenyalcin deleted the improvements branch October 5, 2023 15:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add yaml outputs of resources to streaming logs and add import&update steps to uptest #136

Add yaml outputs of resources to streaming logs and add import&update steps to uptest #136

sergenyalcin commented Sep 28, 2023 •

edited

Loading

ulucinar left a comment

ulucinar Oct 5, 2023

ulucinar Oct 5, 2023

ulucinar Oct 5, 2023

ulucinar Oct 5, 2023

ulucinar Oct 5, 2023

ulucinar Oct 5, 2023

ulucinar Oct 5, 2023

ulucinar Oct 5, 2023

ulucinar Oct 5, 2023

ulucinar Oct 5, 2023

ulucinar Oct 5, 2023

	- script: ${KUBECTL} get --namespace {{ $resource.Namespace }} {{ $resource.KindGroup }}/{{ $resource.Name }} -o yaml && ${KUBECTL} wait {{ $resource.KindGroup }}/{{ $resource.Name }} --for=condition={{ $condition }} --timeout 10s --namespace {{ $resource.Namespace }}
	- script: ${KUBECTL} wait {{ $resource.KindGroup }}/{{ $resource.Name }} --for=condition={{ $condition }} --timeout 10s --namespace {{ $resource.Namespace }}

	- command: ${KUBECTL} get managed -o yaml
	- script: ${KUBECTL} get managed -o yaml \|\| ${KUBECTL} get claim --all-namespaces -o yaml

-		if updateParameter := os.Getenv("UPTEST_UPDATE_PARAMETER"); updateParameter != "" {
+		updateParameter := ... // get value from annotation
+		if updateParameter == "" {
+		   updateParameter = os.Getenv("UPTEST_UPDATE_PARAMETER")
+		}
+		if updateParameter != "" {
+		...

		- command: ${KUBECTL} --subresource=status patch {{ $resource.KindGroup }}/{{ $resource.Name }} --type=merge -p '{"status":{"conditions":[]}}'
		- script: ${KUBECTL} annotate {{ $resource.KindGroup }}/{{ $resource.Name }} uptest-old-id=$(${KUBECTL} get {{ $resource.KindGroup }}/{{ $resource.Name }} -o=jsonpath='{.status.atProvider.id}') --overwrite

	//go:embed 01-assert.yaml.tmpl
	//go:embed 01-assert-update.yaml.tmpl

Add yaml outputs of resources to streaming logs and add import&update steps to uptest #136

Add yaml outputs of resources to streaming logs and add import&update steps to uptest #136

Conversation

sergenyalcin commented Sep 28, 2023 • edited Loading

Description of your changes

How has this code been tested

ulucinar 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

sergenyalcin commented Sep 28, 2023 •

edited

Loading