Conversation
ranatrk
force-pushed
the
suspend-annotations
branch
2 times, most recently
from
6be5dea to
d56f07f
Compare
|
Lets add this little snipped to the makefile and run it to fix the ci lint job |
| t.Errorf("expected annotation weave.works/suspended-by to be set to the principal %s", principalID) | ||
| } | ||
| } else { | ||
| t.Errorf("expected annotation weave.works/suspended-by not found for %s", name.String()) |
There was a problem hiding this comment.
You don't need to call .String() because...
https://pkg.go.dev/fmt
If the format (which is implicitly %v for Println etc.) is valid for a string (%s %q %v %x %X), the following two rules apply:
4. If an operand implements the error interface, the Error method will be invoked to convert the object to a string, which will then be formatted as required by the verb (if any).
5. If an operand implements method String() string, that method will be invoked to convert the object to a string, which will then be formatted as required by the verb (if any).
| for _, tt := range tests { | ||
| name := types.NamespacedName{Name: tt.obj.GetName(), Namespace: ns.Name} | ||
| g.Expect(checkSpec(t, k, name, tt.obj)).To(BeFalse()) | ||
| g.Expect(checkSpec(t, k, principalID, name, tt.obj, false)).To(BeFalse()) |
There was a problem hiding this comment.
I'd probably have split this into to two expectations rather than piggy-backing on top of checkSpec ?
There was a problem hiding this comment.
it was to avoid having to add a switch and getting the objects for each type again, but It can be split if that makes it cleaner/clearer as a test
There was a problem hiding this comment.
I don't think you need to add a switch?
You've got a client.Object https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/client#Object which embeds https://pkg.go.dev/k8s.io/apimachinery/pkg/apis/meta/v1#Object ?
|
|
||
| changeSuspendAnnotations(obj, msg.Suspend, msg.Comment, principal) | ||
|
|
||
| if msg.Suspend { |
There was a problem hiding this comment.
A couple of things here...
weave-gitops/core/server/suspend.go
Lines 15 to 19 in 35231b8
We could reuse the principal from the first call in the second?
Also, we could include the "principalID" in the log.Info so that the user who does it is logged out?
We could also include the name and namespace, and maybe even Kind of the resource, so that the logs would be searchable.
"Who unlocked the GitRepository test/test"?
Bonus points for including the Cluster name :-)
There was a problem hiding this comment.
the name,namespace, and kind are already included . I'll add the principal to the logs 👍
weave-gitops/core/server/suspend.go
Lines 44 to 49 in 35231b8
Remove unused context
generate proto on linux
Remove checkSpec and add getting unstructured object Check for spec.suspend value and suspend annotations in unstructured object
ranatrk
force-pushed
the
suspend-annotations
branch
from
a9a394b to
72848f0
Compare
3 participants
Closes BE part of weaveworks/weave-gitops-enterprise#3546
What changed?
Add suspended-by and suspended-comment annotations to objects being suspended
Remove suspended-by and suspended-comment annotations from objects being resumed
Why was this change made?
To log who/what performed the suspending action on the resource
How was this change implemented?
Adding the annotations needed indicating
suspended-byandsuspended-commentthat could possibly include extra details regarding the suspensionHow did you validate the change?
Release notes
Documentation Changes