-
Notifications
You must be signed in to change notification settings - Fork 73
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
apply generic update #1078
apply generic update #1078
Conversation
/retest known flaky test SANDBOX-836 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice improvement!
test/e2e/parallel/space_test.go
Outdated
s, err := hostAwait.UpdateSpace(t, space.Name, func(s *toolchainv1alpha1.Space) { | ||
s.Spec.TargetCluster = "unknown" | ||
}) | ||
s, err := For(t, hostAwait.Awaitility, &toolchainv1alpha1.Space{}). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait.For()
would look better then just For()
. But it's cosmetic :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed! Addressed, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice 🤩
Thanks for this simplification and removal of the duplications 🥇 🙏
@@ -84,7 +84,7 @@ func TestCreationOfUserAndIdentityIsSkipped(t *testing.T) { | |||
t.Run("user and identity stay there when user is deactivated", func(t *testing.T) { | |||
// when | |||
userSignup, err := wait.For(t, hostAwait.Awaitility, &toolchainv1alpha1.UserSignup{}). | |||
Update(signup.Name, | |||
Update(signup.Name, hostAwait.Namespace, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seeing all these changes, I'm wondering if we really need to provide the namespace explicitly? couldn't it be read from the provided "awaitility" instance?
Just to be clear, it's fine to keep it as it is for now and address it later if we figure out that it would simplify the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were some update functions like UpdateServiceAccount
, UpdateSpaceRequest
, and UpdateConfigMap
that had the namespace as input, that's why I changed a little the generic update function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I see 👍 Thanks for clarification
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexeykazakov, MatousJobanek, rsoaresd The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Quality Gate passedIssues Measures |
Description
This PR cleans up all the update functions that are very similar to the
Update
generic function, addressing the necessary changes.Please, note that I did not clean up
UpdateSpaceBindingRequest
becauseUpdateTestUpdateSpaceBindingRequest
returns err when there is an error while updating, it is needed for this assertionIssue ticket number and link
SANDBOX-838