Skip to content

Change internal ID to UUID in user disable event #11824

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Change internal ID to UUID in user disable event #11824

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions api/src/main/java/org/apache/cloudstack/api/command/admin/user/DisableUserCmd.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,12 +78,12 @@ public long getEntityOwnerId() {

@Override
public String getEventDescription() {
return "disabling user: " + getId();
return "disabling user: " + this._uuidMgr.getUuid(User.class, getId());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@erikbocks , can we make
this._uuidMgr.getUuid(User.class, getId()); a more generic event utility? It also concerns https://github.com/apache/cloudstack/pull/11649/files though it was not changed there. I am pretty sure we can find more places. cc @vishesh92 @bernardodemarco

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DaanHoogland, I did not understand what "more generic event utility" means. For me, the UUIDManager.getUUID() method is as generic as it can be. Could you try to explain a little more of what is your idea?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is a good idea and good start
can you expand this PR to support other resources, for example user (besides disable), account, domain, vm ,network, volume, etc ?
@erikbocks

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DaanHoogland, I did not understand what "more generic event utility" means. For me, the UUIDManager.getUUID() method is as generic as it can be. Could you try to explain a little more of what is your idea?

have the method getId() be replaced by a method getUuid() that takes care of it, for instance. We are sure to have to do a lot of replacements so I have no real preference, but having three dots (.), two method calls and a type passing seems not as generic as we can make it. I do not think we should perse do it in one of these two PRs, this and #11649) but let’s think about it. We are sure to have more occurrences like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think we should perse do it in one of these two PRs, this and #11649) but let’s think about it. We are sure to have more occurrences like this.

Sure. Then, I will map an issue to handle this properly and try to work on it later.

}

@Override
public void execute() {
CallContext.current().setEventDetails("UserId: " + getId());
CallContext.current().setEventDetails("User ID: " + this._uuidMgr.getUuid(User.class, getId()));
UserAccount user = _regionService.disableUser(this);

if (user != null) {
Expand Down
Loading