apiextensions/composite: list unready resources in condition message #4565

sttts · 2023-09-04T15:46:22Z

Description of your changes

Set the Ready condition message for reason Creating to be the first 3 unready resources. They are sorted in the composition and hence give a stable message.

Fixes #3441.

I have:

Read and followed Crossplane's contribution process.
Added or updated unit and E2E tests for my change.
Run make reviewable to ensure this PR is ready for review.
~~Added backport release-x.y labels to auto-backport this PR, if necessary.~~
~~Opened a PR updating the docs, if necessary.~~

internal/controller/apiextensions/composite/reconciler.go

phisco · 2023-09-06T09:01:55Z

looks like this is addressing #3441, am I wrong?

sttts · 2023-09-06T09:52:48Z

looks like this is addressing #3441, am I wrong?

Yes. Added to the description.

internal/controller/apiextensions/composite/reconciler_test.go

Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.io>

internal/controller/apiextensions/composite/reconciler.go

Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.io>

pedjak · 2023-09-08T13:37:23Z

internal/controller/apiextensions/claim/reconciler.go

-		log.Debug("Composite resource is not yet ready")
-		record.Event(cm, event.Normal(reasonBind, "Composite resource is not yet ready"))
+	if cpReady := cp.GetCondition(xpv1.TypeReady); !resource.IsConditionTrue(cpReady) {
+		log.Debug("Composite resource is not yet ready", "reason", cpReady.Reason, "message", cpReady.Message)


would it be helpful to add to the log the composite name as well?

Could be. Not caring about the debug logs though. They are useless anyway. If we cared, we would think about info level ones.

Note that the claim should be part of the log. The claim includes the composition reference.

pedjak · 2023-09-08T13:37:47Z

internal/controller/apiextensions/claim/reconciler.go

+	if cpReady := cp.GetCondition(xpv1.TypeReady); !resource.IsConditionTrue(cpReady) {
+		log.Debug("Composite resource is not yet ready", "reason", cpReady.Reason, "message", cpReady.Message)
+		if msg := cpReady.Message; msg != "" {
+			record.Event(cm, event.Normal(reasonBind, fmt.Sprintf("%s: %s", Waiting().Message, msg)))


same question as above

pedjak

LGTM, - I would add also composite name to the log/events, but not blocking on it

jbw976

Your continued efforts to improve the dev experience are really appreciated @sttts!! Just one question about how we handle resources that potentially don't have a name 🤔

jbw976 · 2023-09-09T02:11:58Z

internal/controller/apiextensions/composite/reconciler.go

-		xr.SetConditions(xpv1.Creating())
+		names := make([]string, len(unready))
+		for i, cd := range unready {
+			names[i] = cd.ResourceName


is ResourceName guaranteed to be a non empty string? https://github.com/crossplane/crossplane/blob/master/internal/controller/apiextensions/composite/reconciler.go#L628-L633 is making think that it doesn't have that guarantee - would we want to do something different with the name here, e.g. fall back to its index number? 🤔

we should print the group kind and name. I was there before actually, then thought that the names are what ppl refer to. Now you are (rightfully) saying they are optional 🙂

actually, kind and name are not guaranteed either. Especially, with the Kubernetes provider these are all objects. The index exists in p&f, not in functions afaik. Looks like we need some heuristic decided by the compositor. What matters here is that we find something pragmatic rather than perfect. Would rather do something imperfect than postponing this topic another 2 years. So +1 putting there the best we have to identify the object. This might also encourage ppl to put proper resource names there to avoid "resource 7" like messages.

Double checked: I think we are good. For p&f, anonymous resources get %d as name. I have made that a little prettier via resource %d now. In functions, anonymous resources are not allowed:

The PTFComposer does not support anonymous templates

@jbw976 ptal

sounds good @sttts, thanks for looking deeper into that! ✅

…ymous resources Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.io>

negz

Thanks @sttts.

negz · 2023-09-11T20:51:09Z

internal/controller/apiextensions/composite/reconciler.go

 	// Note that this 'Composition' will be derived from a
 	// CompositionRevision if the relevant feature flag is enabled.


This comment is stale - revisions are GA and can't be disabled. Not related to this PR but it would be nice to update if doing so is easy.

Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.io>

Git didn't detect any merge conflicts, but the resulting code didn't build. Signed-off-by: Nic Cope <nicc@rk0n.org>

sttts requested review from turkenh and a team as code owners September 4, 2023 15:46

sttts requested a review from pedjak September 4, 2023 15:46

sttts force-pushed the sttts-compositor-messages branch from 451d4d6 to 58cded0 Compare September 4, 2023 16:48

phisco reviewed Sep 5, 2023

View reviewed changes

internal/controller/apiextensions/composite/reconciler.go Outdated Show resolved Hide resolved

sttts force-pushed the sttts-compositor-messages branch from 58cded0 to 4c9a77a Compare September 6, 2023 11:25

phisco approved these changes Sep 6, 2023

View reviewed changes

pedjak requested changes Sep 6, 2023

View reviewed changes

internal/controller/apiextensions/composite/reconciler_test.go Outdated Show resolved Hide resolved

sttts added 3 commits September 8, 2023 08:05

apiextensions/composite: list unready resources in condition message

6469152

Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.io>

apiextensions/composite: naming

409506f

Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.io>

apiextensions/composite: emit event 'Selected composition revision'

cf0bb0e

Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.io>

sttts force-pushed the sttts-compositor-messages branch 2 times, most recently from 96340e1 to b7c61f4 Compare September 8, 2023 08:18

pedjak reviewed Sep 8, 2023

View reviewed changes

internal/controller/apiextensions/composite/reconciler.go Outdated Show resolved Hide resolved

sttts force-pushed the sttts-compositor-messages branch from b7c61f4 to cf0bb0e Compare September 8, 2023 12:31

sttts added 2 commits September 8, 2023 14:35

Bump crossplane-runtime

60b9d55

Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.io>

Use crossplane-runtime First/StableNAndSomeMore helpers

63b1c99

Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.io>

sttts requested a review from a team as a code owner September 8, 2023 12:39

pedjak reviewed Sep 8, 2023

View reviewed changes

pedjak approved these changes Sep 8, 2023

View reviewed changes

sttts force-pushed the sttts-compositor-messages branch from 601e91d to 018d42e Compare September 8, 2023 15:34

jbw976 reviewed Sep 9, 2023

View reviewed changes

sttts force-pushed the sttts-compositor-messages branch from 420ca7f to 63b1c99 Compare September 9, 2023 07:49

apiextensions/composite: use more readable 'resource 7' name for anon…

f37c585

…ymous resources Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.io>

negz approved these changes Sep 11, 2023

View reviewed changes

apiextensions/composite: fix stale revision comment

8d9a72c

Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.io>

phisco approved these changes Sep 12, 2023

View reviewed changes

phisco merged commit d7542dd into crossplane:master Sep 12, 2023
16 checks passed

phisco mentioned this pull request Sep 12, 2023

chore(deps): update dependency golangci/golangci-lint to v1.54.2 (master) #4508

Merged

1 task

negz added a commit to negz/crossplane that referenced this pull request Sep 13, 2023

Fix broken build after rebasing on crossplane#4565

418d486

Git didn't detect any merge conflicts, but the resulting code didn't build. Signed-off-by: Nic Cope <nicc@rk0n.org>

negz added a commit to negz/crossplane that referenced this pull request Sep 13, 2023

Fix broken build after rebasing on crossplane#4565

1d3957c

Git didn't detect any merge conflicts, but the resulting code didn't build. Signed-off-by: Nic Cope <nicc@rk0n.org>

negz added a commit to negz/crossplane that referenced this pull request Sep 13, 2023

Fix broken build after rebasing on crossplane#4565

b8a9043

Git didn't detect any merge conflicts, but the resulting code didn't build. Signed-off-by: Nic Cope <nicc@rk0n.org>

negz mentioned this pull request Oct 2, 2023

Selected composition revision: %!s(MISSING) #4708

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

apiextensions/composite: list unready resources in condition message #4565

apiextensions/composite: list unready resources in condition message #4565

sttts commented Sep 4, 2023 •

edited

phisco commented Sep 6, 2023

sttts commented Sep 6, 2023

pedjak Sep 8, 2023

sttts Sep 8, 2023

sttts Sep 8, 2023

pedjak Sep 8, 2023

pedjak left a comment

jbw976 left a comment

jbw976 Sep 9, 2023

sttts Sep 9, 2023

sttts Sep 9, 2023

sttts Sep 9, 2023

jbw976 Sep 12, 2023

negz left a comment

negz Sep 11, 2023

sttts Sep 11, 2023

		// Note that this 'Composition' will be derived from a
		// CompositionRevision if the relevant feature flag is enabled.

apiextensions/composite: list unready resources in condition message #4565

apiextensions/composite: list unready resources in condition message #4565

Conversation

sttts commented Sep 4, 2023 • edited

Description of your changes

phisco commented Sep 6, 2023

sttts commented Sep 6, 2023

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

pedjak left a comment

Choose a reason for hiding this comment

jbw976 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

negz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Sep 4, 2023 •

edited