-
Notifications
You must be signed in to change notification settings - Fork 26
MEN-8649: Clean OS services from tenantadm related code 🧹 #904
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
base: MEN-8649
Are you sure you want to change the base?
Conversation
87094c2
to
cf7fe3c
Compare
Ticket: MEN-8649 Signed-off-by: Bahaa Aldeen Ghazal <[email protected]>
Ticket: MEN-8649 Signed-off-by: Bahaa Aldeen Ghazal <[email protected]>
cf7fe3c
to
0574a79
Compare
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.
The changes looks good to me, great job!
I created a new upstream branch as we also need to merge these changes to enterprise before committing it to main
.
func (rl *RedisCache) Throttle( | ||
ctx context.Context, | ||
rawToken string, | ||
l ratelimits.ApiLimits, | ||
tid, | ||
id, | ||
idtype, | ||
url, | ||
action string, | ||
) (string, error) { |
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.
Throttle
but without the "Throttling" part 🙃
// in that case we need to wipe identity data from the context | ||
ctx = identity.WithContext(ctx, nil) | ||
} | ||
ctx = identity.WithContext(ctx, nil) |
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.
This hack should be unnecessary after the migration go gin-gonic, right?
That is, if there's no identity middleware for this API.
err := ua.cTenant.UpdateUser(ctx, | ||
idty.Tenant, | ||
id, | ||
&tenant.UserUpdate{ | ||
Name: string(userUpdate.Email), | ||
}, | ||
ua.clientGetter()) |
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.
Wow, this seems like a leftover. This should never happen under any circumstances since we removed duplicating users from tenantadm service. 📝
Ticket: MEN-8649
Tried to not remove too much to not make OS even more different from enterprise.